Allowing complete math overrides #609
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## fix-config-desync #609 +/- ##
=====================================================
+ Coverage 95.93% 95.94% +0.01%
=====================================================
Files 24 24
Lines 3638 3651 +13
Branches 736 735 -1
=====================================================
+ Hits 3490 3503 +13
Misses 84 84
Partials 64 64 |
irm-codebase
changed the title
irm-codebase
force-pushed
the
feature-base-math-override
branch
2 times, most recently
from
e47e12f to
e6e3d44
Compare
There was a problem hiding this comment.
@brynpickering following your comments on #606, I decided to only skip the base math, for now.
The schema approach remains the same for now, since modifying this is likely to be quite a lengthy update since MODEL_SCHEMA is a global and not a Model attribute.
Here are a few comments to aid with the review. Let me know what you think!
There was a problem hiding this comment.
Moved this functionality to model.py because it does not really interact with the backend in any way.
Keeps the code a bit leaner.
pyproject.toml
Outdated
There was a problem hiding this comment.
Enabled some extra linting help for pytest.
It already helped in removing some small issues in some files for this PR.
There was a problem hiding this comment.
New boolean to specify if the base math is enabled in the model. The core of this PR.
| @@ -188,9 +188,6 @@ def _init_from_model_def_dict( | |||
| model_config.union(model_definition.pop("config"), allow_override=True) | |||
|
|
|||
| init_config = update_then_validate_config("init", model_config) | |||
| # We won't store `init` in `self.config`, so we pop it out now. | |||
There was a problem hiding this comment.
I am now allowing config.init to remain in model._model_data.attrs.
I think it makes the model creation more transparent, particularly because the added math files specified are kept.
|
|
||
| log_time( | ||
| LOGGER, | ||
| self._timings, | ||
| "model_data_creation", | ||
| comment="Model: preprocessing stage 2 (model_data)", | ||
| ) | ||
|
|
||
| math, applied_math = self._math_init_from_yaml(init_config) |
There was a problem hiding this comment.
The new _math functions follow a functional programming approach for two reasons:
- easier to write tests for this style
- is explicit
I can change this to an implicit OOP approach if need-be.
| return base_math | ||
| return (math, applied_math) | ||
|
|
||
| def _math_update_with_mode( |
There was a problem hiding this comment.
This is the function I removed from the backend. Works in the same way, except that it returns the updated math instead of directly modifying it.
Also, I made sure to avoid the use of .copy() unless absolutely needed. Should be more efficient in cases were no modification is necessary.
tests/test_backend_pyomo.py
Outdated
There was a problem hiding this comment.
Most of this file is linter updates. Run mode tests were moved to a different test file for consistency.
irm-codebase
changed the title
|
Turns out our data saving steps have some unexpected bugs. Opened #614 to deal with them. |
|
Converting to draft because #614 is a blocker to this PR. |
Fixes #606
Summary of changes in this pull request
config.init.base_math) that will tell the model to skip base math if set toFalse.initorbuild). This is to prevent unexpected surprises if users save an initialized model without building it.Reviewer checklist