Rework math handling #639
Rework math handling #639
Conversation
|
Tried to improve Ultimately I decided it was not worth the hassle, but this leads to unnecessary code ( |
|
@brynpickering requesting a preeliminary review of this, with some open questions: |
There was a problem hiding this comment.
Nice cleanup. Just a few comments:
- math documentation building isn't really a postprocessing step, it's a parallel process to building/solving the optimisation problem. Not sure where it should go, tbh.
- This is getting us one step closer to removing math from
calliope.Modelentirely, and I think I'm in favour of that. It just requires moving thevalidating_math_stringsmethod to somewhere more suitable. - I like being able to add a math dictionary and not just a reference to a math YAML. It's effectively equivalent to adding
scenariosvsoverride_dictatcalliope.Modelinstatiation. Ideally we'd add this tocalliope.Model.buildand allow that dict to completely replace the math dict or to be added as another override. If the approach matchesscenarios/override_dictthenadd_mathlist would be applied first, followed by themath_dict. It just then needs a flag for ignoringmath/base.yaml(/math/plan.yaml). - Shall we use
CalliopeMathas the class name? Many libraries seem to do this to avoid name clashes when using their lib as a dependency.
| model_def_with_overrides["nodes"] = model_def_with_overrides["locations"] | ||
| del model_def_with_overrides["locations"] |
There was a problem hiding this comment.
| model_def_with_overrides["nodes"] = model_def_with_overrides["locations"] | |
| del model_def_with_overrides["locations"] | |
| model_def_with_overrides["nodes"] = model_def_with_overrides.pop("locations") |
| def _combine_overrides(overrides: AttrDict, scenario_overrides: list): | ||
| combined_override_dict = AttrDict() | ||
| for override in scenario_overrides: | ||
| try: |
There was a problem hiding this comment.
I tend to find try/except to be less readable. if override not in overrides is much more explicit
src/calliope/model.py
Outdated
| @@ -561,8 +526,8 @@ def validate_math_strings(self, math_dict: dict) -> None: | |||
| """ | |||
There was a problem hiding this comment.
I guess it should move to the parsing module? It just needs a list of possible parameters passed to it so that it can use that in the parsing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #639 +/- ##
==========================================
- Coverage 95.98% 95.95% -0.04%
==========================================
Files 26 29 +3
Lines 3985 4025 +40
Branches 838 775 -63
==========================================
+ Hits 3825 3862 +37
- Misses 70 72 +2
- Partials 90 91 +1
|
- clustering had representative days that didn't represent themselves
|
@irm-codebase I've made a bunch of changes, partly following offline discussions between us.
|
|
@brynpickering fantastic! It's a lot of changes, so I'll review them once I'm back this Monday. |
There was a problem hiding this comment.
@brynpickering do you still consider this not a postprocessing feature after going through the changes?
should we change something here?
|
@brynpickering what's next for this feature? |
|
@irm-codebase I'm unsure as to the best approach to this now, especially if we move math back to the model instatiation step when introducing parameters. Still, there's lots of useful updates here, so perhaps you could clean up the conflicts and assuming @sjpfenninger is also happy with it we can merge and deal with where Could I ask that you also resolve all the comments that are now outdated? There's lots on here that I have the impression are still open issues but may have been dealt with by your updates. |
|
@brynpickering I think that's the best approach. |
irm-codebase
force-pushed
the
rework-math
branch
from
527dfa9
to
d024911
Compare
|
@brynpickering I've updated everything, and tests pass.
Seems related to breaking changes in 0.6 (https://astral.sh/blog/ruff-v0.6.0#migrating-to-v06) Have you faced this issue before? |
|
Yes, I'm having the same issues. It is just a mismatch in the version of ruff you're using on autosave vs the one pre-commit has updated to. Best is to use |
|
That's odd... that's the configuration I am using. I even tried updating the pre-commit hook and ruff to 0.6.8 (the newest), and it still fails. Let me know if you find an approach that works. |
|
Weird, works fine for me having updated ruff to v0.6 in my calliope dev env |
Partially fixes #608 #619
Summary of changes in this pull request
This PR aims to improve the way in which math is handled across a model init->build->solve->postprocess pipeline.
MathDocumentationis now inpostprocess.Modelno longer includes it (instead, it's an input to it).model._model_def_path->model._def_path: the processing of this was made clearer to remove unnecessary code during__init__. As a result,load.pyis nowscenarios.py, which is what it was really doing all along.ModelMathintroduced to handle math better. Thispreprocessclass contains the following:_model_data.attrs["applied_additional_math"]was eliminated in favor ofmath.history, which is also saved/read from netCDFs.model._model_data.attrsno longer contains"math".base.yaml->plan.yaml. This is to avoid extra logic when checking math modes)... plus its clearer, imo.Additional discussion
Stuff that I could add to this PR very easily, with permission.
Removal of double
mathobjectsFeatured in the PR!
Removingmodel.mathis very possible now, because all the logic needed to process files is contained inModelMath. Users can still reach it if we declare a@propertythat returnsmodel.backend.math.data, or a warning if the backend has not been built / initialized.I have not done this because it needs discussing (e.g., should we partially initialise the backend duringinit?). This resulted in some additional attributes to theModelBackendGenerator, which can be easily removed.Enabling no math ('clean') models
Featured in the PR!
This PR also allows us to fulfill #606 very easily. I believe that changingconfig.init.add_math::default::[]toconfig.init.added_math::default::['plan']is the most transparent way and the easiest to maintain.However, this might lead to some users accidentally omittingplanmath if they add their own math... but this could hint at a documentation problem on our side.Another option is to addconfig.init.default_math::default:true, but it's not my favorite because it seems intransparent...Reviewer checklist