-
Notifications
You must be signed in to change notification settings - Fork 127
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
fix: Adding non-default loggables explicitly. #1331
Conversation
Now `hoomd.logging.Logger.add` will add non-default loggables when explicitly asked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small fix in the tests here, but I would like to see some more complete tests for the Logger
class's behavior.
Just renamed a fixture.
* Test multiple constructor arguments in Logger * Add some additional checks to existing tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small question, then this should be ready to merge.
@cbkerr, please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had trouble reviewing because I couldn't parse the docstrings. I made a commit with my re-interpretation of them. Please correct if it's wrong.
One problem is that "pass over" can mean "skip, don't give to logger" or "give to the logger"
I think the logic in the tests makes sense
hoomd/logging.py
Outdated
logged by "default", defaults to ``True``. This mostly means that | ||
performance centric loggable quantities will be passed over when | ||
logging when false. | ||
logged by "default". Defaults to ``True``. If ``False``, loggable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the quotes on by "default"
?
Where is this "default" specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is specified by the specific loggable. We could use italic. I was just trying to emphasize the word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think emphasizing it is confusing because the parameter also contains "default". Would we lose much by omitting this bit?
hoomd/logging.py
Outdated
``only_default`` flag is mainly a convenience by allowing quantities not | ||
commonly logged (but available) to be passed over unless explicitly asked | ||
for. You can override the ``only_default`` flag by explicitly listing the | ||
``only_default`` flag affects performance by controlling whether available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbkerr only_default
doesn't really effect performance. It is just to prevent quantities that may not be commonly desired from automatically being added on Logger.__iadd__
and Logger.add
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see the documentation was completely misleading here... I think I meant show not slow...
@cbkerr Let us know if these changes addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra details in the docstrings help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix readthedocs and precommit failures.
The RTD failure came from the auto upgrade to 5.0.0 sphinx-doc, specifically:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Description
Fixes #1328, by adding
force_quantities
to internal loggable filtering.Motivation and context
Resolves #1328
How has this been tested?
A test will be added soon.
Change log
Checklist:
sphinx-doc/credits.rst
) in the pull request source branch.