fix: Adding non-default loggables explicitly. #1331
Conversation
Now `hoomd.logging.Logger.add` will add non-default loggables when explicitly asked.
b-butler
requested review from
tommy-waltmann and
cbkerr
and removed request for
a team
Just renamed a fixture.
* Test multiple constructor arguments in Logger * Add some additional checks to existing tests
joaander
added
the
validate
|
@cbkerr, please review. |
There was a problem hiding this comment.
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.
Why the quotes on by "default"?
Where is this "default" specified?
There was a problem hiding this comment.
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.
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.
@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.
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. |
|
The RTD failure came from the auto upgrade to 5.0.0 sphinx-doc, specifically:
|
Description
Fixes #1328, by adding
force_quantitiesto 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.