-
Notifications
You must be signed in to change notification settings - Fork 9
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
New BoundsChecker implementation #409
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #409 +/- ##
===========================================
+ Coverage 96.27% 96.50% +0.22%
===========================================
Files 34 36 +2
Lines 2659 2748 +89
===========================================
+ Hits 2560 2652 +92
+ Misses 99 96 -3 ☔ 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.
This looks, in principle, good. I am just starting to wonder, if everything is made a class, won't there, at some point, be a significant runtime penalty? But maybe it's an outdated clichee (or was never true in the first place for sequential codes) that OOP makes things slower, runtime-wise.
I worry about that a bit. This PR is a small class that is basically just used to apply some numpy functions to potentially large arrays. I doubt it is significantly slower than the original, if at all. I think there is a minor cost to accessing by attribute, but it's a relatively small number of attributes that are accessed once per variable in the whole of an analysis. |
I agree that this one instance will will not make a bit difference, it was more a general observation about the way the code develops. |
variable name keys using the ``update`` method. | ||
""" | ||
|
||
# TODO - think about these argument names - some unnecessarily terse. |
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.
What plan do you have for renaming these vars?
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.
Good question - I think once the API structural changes have been pushed through, then review the function signatures for variable name and synchronise them across the code base. So a polishing step once we've put it all back together.
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.
Looks good
Description
This PR replaces
core.utilities.bounds_checker
with a user-modifiable and extendableBoundsChecker
class. This does add one more argument to some class signatures but we plan to reorganise signatures (#394).The main advantages are that:
PModelEnvironment
The implementation:
Bounds
dataclass used to define bounds for a particular variable.BoundChecker
class with default bounds for core variables that acts like a library for bounds checking.BoundsChecker().check("tc", np.array([10, 1000])
, which will check that these alleged temperature data fall within the configured bounds.Fixes #408
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks