-
Notifications
You must be signed in to change notification settings - Fork 13
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
Rename setup_classes & include creation_date in ExperimentInfo #1344
Conversation
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 official interface for using EvalSetup
is:
from pyaerocom.aeroval import EvalSetup
Therefore, renaming setupclasses to setup_classes is fine here. It would be good if the test-cases could use the official interface rather than the internal access-path.
The documentation for EvalSetup needs to been fixed:
pyaerocom/docs/api-aeroval.rst
Line 14 in be7aeef
.. automodule:: pyaerocom.aeroval.setupclasses |
Can creation_date have time-resolution e.g. https://stackoverflow.com/questions/10286204/what-is-the-right-json-date-format
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main-dev #1344 +/- ##
=========================================
Coverage 78.97% 78.97%
=========================================
Files 136 136
Lines 20815 20817 +2
=========================================
+ Hits 16438 16440 +2
Misses 4377 4377
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Please use the json-datetime format and utc.
Please use the official way to import EvalSetup where possible.
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.
I just got a small idea on how to hide the setup_classes
in user/test-space even more, not crucial, so I approve the PR and leave it to you if you want to change or just merge.
@@ -113,7 +113,7 @@ def test_ExperimentOutput(): | |||
def test_ExperimentOutput_error(): | |||
with pytest.raises(ValueError) as e: | |||
ExperimentOutput(None) | |||
assert str(e.value) == "need instance of <class 'pyaerocom.aeroval.setupclasses.EvalSetup'>" | |||
assert str(e.value) == "need instance of <class 'pyaerocom.aeroval.setup_classes.EvalSetup'>" |
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.
To avoid hardcoding class-pathes, it could be written as:
f"need instance of {EvalSetup}"
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 this ValueError.value
comes from
pyaerocom/pyaerocom/_lowlevel_helpers.py
Line 105 in be7aeef
raise ValueError(f"need instance of {self._type}") |
so it would be a little tricky to change just in this case.
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 just meant replace
assert str(e.value) == "need instance of <class 'pyaerocom.aeroval.setupclasses.EvalSetup'>"
with
assert str(e.value) == f"need instance of {EvalSetup}"
This would not need any changes to _lowlevel_helpers.py. The string expansion of {self._type}
and {class}
is the the same.
Change Summary
setupclasses.py
tosetup_classes.py
to follow Python module naming standard of splitting words with underscorescreation_date
attribute toExperimentInfo
. Defaults todatetime.today().strftime("%Y-%m-%d")
but can also be provided if a pyaeroval user wants to modified it.Related issue number
Close https://github.com/metno/AeroToolsIssues/issues/75
Checklist