Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update PushLogQuantity and add value setter for user-defined logging scalars #74

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

thomasgibson
Copy link

This PR is a direct continuation of #69.

Examples have been updated to use the built-in PushLogQuantity object, and a simple value setter function (set_quantity_value) is importable which allows for explicit updating.

@thomasgibson
Copy link
Author

@inducer This PR addresses all your comments in #69 --- so that PR is safe to close. I had issues with local forks so I just felt it easier to reopen with a new branch. I think everything in here should be (hopefully) straightforward :)

Copy link
Collaborator

@inducer inducer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I think we're converging on an interface that makes sense. Some thoughts on that below.

Comment on lines +144 to +148
def set_value(self, value: Any) -> None:
"""Set the logged quantity value."""
raise NotImplementedError(
f"set_value not implemented for log quantity {self.name}."
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it avoidable to have set_value in the LogQuantity interface instead of only in PushLogQuantity? Having it breeds an expectation that every LogQuantity support it, which isn't right.

@@ -867,12 +887,13 @@ def add_quantity(self, quantity: LogQuantity, interval: int = 1) -> None:
:param interval: interval (in time steps) when to gather this quantity.
"""

def add_internal(name, unit, description, def_agg):
def add_internal(name, unit, description, def_agg, set_value=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're standardizing set_value in PushLogQuantity, I'm no longer sold on the value of passing in set_value.

@@ -251,11 +262,18 @@ def __init__(self, dt: Optional[float], name: str, unit: str = None,


class PushLogQuantity(LogQuantity):
"""A loggable scalar whose value can be updated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should comment a bit on how the LogQuantity interface is a "pull" model generically, and how this offers "push" capability over it. What happens to redundant "set" attempts? What if nothing is pushed in a cycle?

@@ -1498,6 +1521,15 @@ def add_run_info(mgr: LogManager) -> None:
mgr.set_constant("date", strftime("%a, %d %b %Y %H:%M:%S %Z", localtime()))
mgr.set_constant("unixtime", time())


def set_quantity_value(mgr: LogManager, name: str, value: Any) -> None:
"""Set a the value of a named LogQuantity.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should narrow this to erroring if that quantity is not a push quantity.

@inducer
Copy link
Collaborator

inducer commented Sep 26, 2021

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants