Add arbitrary user quantities to logging #391
Conversation
MTCam
changed the title
| class LogUserQuantity(LogQuantity): | ||
| """Logging support for arbitrary user quantities.""" | ||
|
|
||
| def __init__(self, name="user_quantity", value=None, user_function=None) -> None: |
There was a problem hiding this comment.
Get rid of the user_function?
There was a problem hiding this comment.
While the user_function is not needed for this particular quantity, it would be required for doing anything more complicated than just setting a simple value. This interface enables the user to specify a F(self._quantity) instead of a simple scalar value, without the need to extend logging with another user-defined quantity class.
There was a problem hiding this comment.
I think passing in a function here might be a bit dangerous - how do you pass the function arguments, and how do you make sure they aren't stale? I'd remove this, and maybe think about creating a new class whenever we have a more clear use case.
There was a problem hiding this comment.
| def __init__(self, name="user_quantity", value=None, user_function=None) -> None: | |
| def __init__(self, name, unit, description="") -> None: |
There was a problem hiding this comment.
I think the burden of implementing the function with all its required data is on the user. This will be needed for anything more complicated than a simple value set. Consider this example, where the user wanted instead to perform the reduction on the inside of the logging class:
def get_domain_max(discr, field):
from grudge.op import nodal_max
return nodal_max(discr, "vol", field)
logmgr.add_quantity(UserLogQuantity(name="cfl", value=current_cfl,
description="inviscid CFL",
user_function=partial(get_domain_max, discr))
def pre_step_loop_body(step, t, dt, state):
if do_viz:
current_cfl = get_inviscid_cfl(blah, blah, blah)
logmgr.set_quantity_value("cfl", current_cfl)To me, this code is better than what we are doing currently (even in this PR) and is superior to the one where the discretization object (or the state, and/or the EOS) are implicitly used to calculate quantities under-the-covers.
mirgecom/logging_quantities.py
Outdated
| self._value = value | ||
| self._uf = user_function | ||
|
|
||
| def set_quantity(self, value) -> None: |
There was a problem hiding this comment.
Allow calling this via
logmgr.set_quantity(name, value)
so that you don't have to lug around the LogQuantity?
There was a problem hiding this comment.
I'll take a crack at this unless this requires @matthiasdiener attention.
There was a problem hiding this comment.
examples/vortex-mpi.py
Outdated
| if logmgr: | ||
| logmgr_add_device_name(logmgr, queue) | ||
| logmgr_add_device_memory_usage(logmgr, queue) | ||
| logmgr_add_many_discretization_quantities(logmgr, discr, dim, | ||
| extract_vars_for_logging, units_for_logging) | ||
| logmgr.add_quantity(LogCFL(discr, eos)) | ||
|
|
||
| logmgr.add_quantity(my_log_quantity) |
There was a problem hiding this comment.
| logmgr.add_quantity(my_log_quantity) | |
| logmgr.add_quantity(LogUserQuantity(name="cfl", value=current_cfl)) |
mirgecom/logging_quantities.py
Outdated
| self._value = value | ||
| self._uf = user_function | ||
|
|
||
| def set_quantity(self, value) -> None: |
There was a problem hiding this comment.
| class LogUserQuantity(LogQuantity): | ||
| """Logging support for arbitrary user quantities.""" | ||
|
|
||
| def __init__(self, name="user_quantity", value=None, user_function=None) -> None: |
There was a problem hiding this comment.
I think passing in a function here might be a bit dangerous - how do you pass the function arguments, and how do you make sure they aren't stale? I'd remove this, and maybe think about creating a new class whenever we have a more clear use case.
| class LogUserQuantity(LogQuantity): | ||
| """Logging support for arbitrary user quantities.""" | ||
|
|
||
| def __init__(self, name="user_quantity", value=None, user_function=None) -> None: |
There was a problem hiding this comment.
| def __init__(self, name="user_quantity", value=None, user_function=None) -> None: | |
| def __init__(self, name, unit, description="") -> None: |
Follows onto #390, and adds an interface for arbitrary user quantities. Demonstrating use with CFL. In this one, the use is free to use the CFL field and visualize it, and the logger does not redundantly calculate it.
fixes #389