Rework math handling #639

irm-codebase · 2024-07-16T10:11:18Z

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.

MathDocumentation is now in postprocess. Model no 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.py is now scenarios.py, which is what it was really doing all along.
ModelMath introduced to handle math better. This preprocess class contains the following:
- math file additions (either user math or pre-defined math).
- math file history checks, in order of addition
- forbidding adding the same file twice (easy to remove, if this would cause trouble)
- math validation against the schema and against external math dictionaries (before adding to the model).
- methods to easily save / read math from netCDF attributes.
_model_data.attrs["applied_additional_math"] was eliminated in favor of math.history, which is also saved/read from netCDFs.
model._model_data.attrs no 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 `math` objects

Removing model.math is very possible now, because all the logic needed to process files is contained in ModelMath. Users can still reach it if we declare a @property that returns model.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 during init?).
This resulted in some additional attributes to the ModelBackendGenerator, which can be easily removed.

Enabling no math ('clean') models

This PR also allows us to fulfill #606 very easily. I believe that changing config.init.add_math::default::[] to config.init.added_math::default::['plan'] is the most transparent way and the easiest to maintain.

However, this might lead to some users accidentally omitting plan math if they add their own math... but this could hint at a documentation problem on our side.

Another option is to add config.init.default_math::default:true, but it's not my favorite because it seems intransparent...

Reviewer checklist

Test(s) added to cover contribution
Documentation updated
Changelog updated
Coverage maintained or improved

…rated)

irm-codebase · 2024-07-18T11:30:30Z

Tried to improve model_math.py by redefining it as a @dataclass. Unfortunately, adding AttrDict as a parameter to a dataclass results in funky behavior. dataclasses.asdict fails, for example.

Ultimately I decided it was not worth the hassle, but this leads to unnecessary code (__eq__, __repr__, etc). Maybe we could improve AttrDict in the future to enable this usage.

irm-codebase · 2024-07-18T15:03:09Z

@brynpickering requesting a preeliminary review of this, with some open questions:

irm-codebase

Few comments to help with the review. Let me know what you think!

docs/hooks/generate_math_docs.py

src/calliope/backend/__init__.py

src/calliope/backend/backend_model.py

src/calliope/backend/latex_backend_model.py

src/calliope/preprocess/model_math.py

irm-codebase · 2024-07-18T21:37:43Z

src/calliope/preprocess/scenarios.py

Removed chunks of code thanks to the init improvements in Model.

tests/conftest.py

tests/test_postprocess_math_documentation.py

irm-codebase · 2024-07-18T21:42:47Z

tests/test_preprocess_model_math.py

+        expected_math.union(user_math, allow_override=True)
+        flat = expected_math.as_dict_flat()
+        assert all(
+            model_math_w_mode_user.data.get_key(i) == flat[i] for i in flat.keys()


Checks every single math combination!

brynpickering

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.Model entirely, and I think I'm in favour of that. It just requires moving the validating_math_strings method 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 scenarios vs override_dict at calliope.Model instatiation. Ideally we'd add this to calliope.Model.build and allow that dict to completely replace the math dict or to be added as another override. If the approach matches scenarios/override_dict then add_math list would be applied first, followed by the math_dict. It just then needs a flag for ignoring math/base.yaml(/math/plan.yaml).
Shall we use CalliopeMath as the class name? Many libraries seem to do this to avoid name clashes when using their lib as a dependency.

docs/hooks/generate_math_docs.py

src/calliope/backend/__init__.py

src/calliope/backend/backend_model.py

brynpickering · 2024-07-19T09:44:52Z

src/calliope/preprocess/scenarios.py

+        model_def_with_overrides["nodes"] = model_def_with_overrides["locations"]
+        del model_def_with_overrides["locations"]


Suggested change

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")

brynpickering · 2024-07-19T09:47:41Z

src/calliope/preprocess/scenarios.py

+def _combine_overrides(overrides: AttrDict, scenario_overrides: list):
+    combined_override_dict = AttrDict()
+    for override in scenario_overrides:
+        try:


I tend to find try/except to be less readable. if override not in overrides is much more explicit

tests/conftest.py

tests/test_preprocess_model_math.py

brynpickering · 2024-07-19T09:57:53Z

src/calliope/model.py

@@ -561,8 +526,8 @@ def validate_math_strings(self, math_dict: dict) -> None:
        """


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 · 2024-07-19T13:58:27Z

Codecov Report

Attention: Patch coverage is 96.08696% with 9 lines in your changes missing coverage. Please review.

Project coverage is 95.98%. Comparing base (4fc6b84) to head (3284a55).
Report is 4 commits behind head on main.

Files	Patch %	Lines
src/calliope/preprocess/scenarios.py	86.66%	4 Missing ⚠️
src/calliope/util/tools.py	66.66%	1 Missing and 1 partial ⚠️
src/calliope/backend/latex_backend_model.py	80.00%	0 Missing and 1 partial ⚠️
src/calliope/model.py	96.77%	0 Missing and 1 partial ⚠️
src/calliope/postprocess/math_documentation.py	96.29%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #639   +/-   ##
=======================================
  Coverage   95.97%   95.98%           
=======================================
  Files          26       29    +3     
  Lines        3980     4014   +34     
  Branches      836      771   -65     
=======================================
+ Hits         3820     3853   +33     
- Misses         70       72    +2     
+ Partials       90       89    -1

Files	Coverage Δ
src/calliope/attrdict.py	`96.48% <100.00%> (ø)`
src/calliope/backend/__init__.py	`100.00% <100.00%> (ø)`
src/calliope/backend/backend_model.py	`97.97% <100.00%> (+0.01%)`	⬆️
src/calliope/backend/expression_parser.py	`93.75% <100.00%> (+0.01%)`	⬆️
src/calliope/backend/gurobi_backend_model.py	`95.66% <100.00%> (ø)`
src/calliope/backend/parsing.py	`96.99% <ø> (ø)`
src/calliope/backend/pyomo_backend_model.py	`98.11% <100.00%> (ø)`
src/calliope/io.py	`96.80% <100.00%> (-0.04%)`	⬇️
src/calliope/preprocess/__init__.py	`100.00% <100.00%> (ø)`
src/calliope/preprocess/model_math.py	`100.00% <100.00%> (ø)`
... and 6 more

- clustering had representative days that didn't represent themselves

brynpickering · 2024-08-02T09:46:28Z

@irm-codebase I've made a bunch of changes, partly following offline discussions between us.

I moved math to only be introduced at the calliope.Model.build step. I think this generally works well.
I updated calliope.Model.math to calliope.Model.applied_math so it's clear that the math has already been applied (since the optimisation problem must have been built!)
I've added math dict application as a build arg and enabled ignoring the mode math with a config.build option (default=false) ignore_mode_math. This could also be flipped to something like include_mode_math (default=true).
I've moved math dict parsing validation to the calliope.Model.build step. It duplicated dict parsing that is done when adding each optimisation component, but does it much quicker (so you can catch math errors quickly for a large model). I've also added a config.build option to activate it (default=true): pre_validate_math_strings.

irm-codebase · 2024-08-02T10:32:33Z

@brynpickering fantastic! It's a lot of changes, so I'll review them once I'm back this Monday.

irm-codebase

Went through the changes. I'm quite happy with them!

I've added some suggestions to avoid bugs catched by mypy, and to improve logic in some parts.

irm-codebase · 2024-08-17T08:15:26Z

src/calliope/backend/backend_model.py

-            "objectives",
-        ]:
+        self._add_all_inputs_as_parameters()
+        if self.inputs.attrs["config"]["build"]["pre_validate_math_strings"]:


irm-codebase · 2024-08-17T08:16:44Z

src/calliope/backend/backend_model.py

            component = components.removesuffix("s")
-            for name in self.math.data[components]:
+            for name, dict_ in self.math.data[components].items():


General note: it'd be better if we used more descriptive names than dict_.
It makes it a bit harder to know what is being fetched and why.

irm-codebase · 2024-08-17T08:18:34Z

src/calliope/backend/backend_model.py

@@ -280,7 +279,7 @@ def _add_component(
                this name must be available in the input math provided on initialising the class.
            component_dict (Tp): unparsed YAML dictionary configuration.
            component_setter (Callable): function to combine evaluated xarray DataArrays into backend component objects.
-            component_type (Literal["variables", "global_expressions", "constraints", "objectives"]):
+            component_type (Literal["variables", "global_expressions", "constraints", "piecewise_constraints", "objectives"]):


You could change Literal... to ORDERED_COMPONENTS_T to make this smaller.
Users would see it though, so a more descriptive name (MATH_COMPONENTS_T?) might be a good idea.

See proposal to move this to a TypedDict in CalliopeMath below.

src/calliope/backend/backend_model.py

irm-codebase · 2024-08-17T08:39:46Z

src/calliope/backend/latex_backend_model.py

@@ -491,7 +483,7 @@ def generate_math_doc(
            ]
            if getattr(self, objtype).data_vars
        }
-        if not components["parameters"]:
+        if "parameters" in components and not components["parameters"]:


Potential bug:
This will also eliminate parameters with False, 0 and similar values.
We may want this due to sparsity, but I am not sure. If you only want to target None, do if not None instead.

irm-codebase · 2024-08-17T08:46:25Z

src/calliope/postprocess/math_documentation.py

@brynpickering do you still consider this not a postprocessing feature after going through the changes?
should we change something here?

irm-codebase · 2024-08-17T09:19:13Z

src/calliope/model.py

+        end_math_list = [] if add_math_dict is None else [add_math_dict]
+        full_math_list = init_math_list + backend_config["add_math"] + end_math_list
+        LOGGER.debug(f"Math preprocessing | Loading math: {full_math_list}")
+        model_math = preprocess.CalliopeMath(full_math_list, self._def_path)


This "math mode salad" is quite hard to follow because of its "mixed value" nature.

init_math_list will have either [] or [str]

the middle (backend_config["add_math"]) will have [] [str]

end_math_list (add_math_dict) will have either None or [dict]

How about:

math_list = [backend_config.get("mode"), backend_config.get("add_math"), add_math_dict] math_list = list(filter(bool, math_list)) # Removes False equivalents None, "", [], {} model_math = preprocess.CalliopeMath(math_list, self._def_path)

irm-codebase · 2024-08-17T09:25:17Z

src/calliope/preprocess/model_math.py

-            self._init_from_dict(math_to_add)
+        self.data: AttrDict = AttrDict(
+            {
+                "variables": {},


Should be TypedDict above?
The backend also refers to through ORDERED_COMPONENTS_T, so maybe the order should be defined here so that we have it in one tidy place.

irm-codebase · 2024-08-17T09:31:52Z

src/calliope/preprocess/model_math.py


    ATTRS_TO_SAVE = ("history", "data")
+    ATTRS_TO_LOAD = ("history",)


Why are we not loading back the math data?
I was doing it because the file structure might have changed, meaning the "rebuild" would fail since the reference directories would be different...

irm-codebase · 2024-08-17T09:37:36Z

src/calliope/preprocess/model_math.py

+    def __repr__(self) -> str:
+        """Custom string representation of class."""
+        return f"""Calliope math definition dictionary with:
+    {len(self.data["variables"])} decision variable(s)
+    {len(self.data["global_expressions"])} global expression(s)
+    {len(self.data["constraints"])} constraint(s)
+    {len(self.data["piecewise_constraints"])} piecewise constraint(s)
+    {len(self.data["objectives"])} objective(s)
+        """


Incorrect tabs trigger a mypy error.

Suggested change

def __repr__(self) -> str:

"""Custom string representation of class."""

return f"""Calliope math definition dictionary with:

{len(self.data["variables"])} decision variable(s)

{len(self.data["global_expressions"])} global expression(s)

{len(self.data["constraints"])} constraint(s)

{len(self.data["piecewise_constraints"])} piecewise constraint(s)

{len(self.data["objectives"])} objective(s)

"""

def __repr__(self) -> str:

"""Custom string representation of class."""

return f"""

Calliope math definition dictionary with:

{len(self.data["variables"])} decision variable(s)

{len(self.data["global_expressions"])} global expression(s)

{len(self.data["constraints"])} constraint(s)

{len(self.data["piecewise_constraints"])} piecewise constraint(s)

{len(self.data["objectives"])} objective(s)

"""

irm-codebase · 2024-08-17T10:34:32Z

src/calliope/backend/expression_parser.py

@@ -788,7 +789,7 @@ def as_array(self) -> xr.DataArray:  # noqa: D102, override
                evaluated = backend_interface._dataset[self.name]
            except KeyError:
                evaluated = xr.DataArray(self.name, attrs={"obj_type": "string"})
-        if "default" in evaluated.attrs:
+        if "default" in evaluated.attrs and pd.notnull(evaluated.attrs["default"]):


pd.notnull here should change.

Generally, pd.isna and pd.notna is preferred over null (https://docs.astral.sh/ruff/rules/pandas-use-of-dot-not-null/)

This will generate odd behavior if there is a list in "default", since it will return an object and the check will pass if there is anything in it.

irm-codebase · 2024-08-24T13:55:30Z

@brynpickering what's next for this feature?
Will we wait until the approach to parameters is set, or should we merge this beforehand?

irm-codebase added 2 commits July 15, 2024 19:28

isolate _def_path, scenario overrides, and math object (not yet integ…

a47e1df

…rated)

Extract math documentation from model file

d2d70ce

irm-codebase marked this pull request as draft July 16, 2024 10:16

irm-codebase changed the title ~~Rework math~~ Rework math handling Jul 16, 2024

irm-codebase added 3 commits July 16, 2024 21:55

add model math tests

3f44bbf

Improve model math tests, remove duplicates from test_core_model

28f8ebb

extended validation function, added logging tests

2872060

irm-codebase added 2 commits July 18, 2024 15:18

Add dict method, remove underscores in attributes

31de31b

code now uses new math object (tests exected to fail)

a6e8c7c

irm-codebase requested review from brynpickering and sjpfenninger July 18, 2024 14:53

all tests passing

fda1ea0

irm-codebase self-assigned this Jul 18, 2024

irm-codebase marked this pull request as ready for review July 18, 2024 20:27

irm-codebase marked this pull request as draft July 18, 2024 20:27

irm-codebase changed the base branch from rework-model-data-handling to main July 18, 2024 20:28

fix docs creation, add changelog

480bcc1

irm-codebase marked this pull request as ready for review July 18, 2024 20:28

irm-codebase mentioned this pull request Jul 18, 2024

Clean and isolate model/backend attributes #635

Closed

10 tasks

irm-codebase commented Jul 18, 2024

View reviewed changes

removed _model_def_dict

6e3e334

brynpickering reviewed Jul 19, 2024

View reviewed changes

brynpickering and others added 2 commits July 19, 2024 11:10

Trigger CI (and minor logging string fix)

8da8dac

PR: now CalliopeMath, better backend init, small fixes

6730409

PR: comment improvements

09b09f7

irm-codebase mentioned this pull request Jul 19, 2024

Ensuring data and model configuration remain in sync at all times (model, backend, backendmath) #619

Open

3 tasks

update changelog

1530b8f

irm-codebase and others added 4 commits July 19, 2024 19:48

Merge branch 'rework-def-dict' into rework-math

41b7fe5

Merge branch 'main' into rework-math

434236d

Post-merge fixes

06efe33

Move math to build step; fix clustering issues

a2ee8f0

- clustering had representative days that didn't represent themselves

irm-codebase commented Aug 17, 2024

View reviewed changes

PR improvements: math components in CalliopeMath, small fixes (#665)

3284a55

irm-codebase mentioned this pull request Sep 8, 2024

Remove examples from documentation search #675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework math handling #639

Rework math handling #639

irm-codebase commented Jul 16, 2024 •

edited

Loading

irm-codebase commented Jul 18, 2024 •

edited

Loading

irm-codebase commented Jul 18, 2024 •

edited

Loading

irm-codebase left a comment

irm-codebase Jul 18, 2024

irm-codebase Jul 18, 2024

brynpickering left a comment •

edited

Loading

brynpickering Jul 19, 2024

brynpickering Jul 19, 2024

brynpickering Jul 19, 2024

codecov bot commented Jul 19, 2024 •

edited

Loading

brynpickering commented Aug 2, 2024

irm-codebase commented Aug 2, 2024

irm-codebase left a comment

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024 •

edited

Loading

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024

irm-codebase Aug 17, 2024

irm-codebase commented Aug 24, 2024

		model_def_with_overrides["nodes"] = model_def_with_overrides["locations"]
		del model_def_with_overrides["locations"]

		@@ -561,8 +526,8 @@ def validate_math_strings(self, math_dict: dict) -> None:
		"""


		ATTRS_TO_SAVE = ("history", "data")
		ATTRS_TO_LOAD = ("history",)

Rework math handling #639

Are you sure you want to change the base?

Rework math handling #639

Conversation

irm-codebase commented Jul 16, 2024 • edited Loading

Summary of changes in this pull request

Additional discussion

Removal of double math objects

Enabling no math ('clean') models

Reviewer checklist

irm-codebase commented Jul 18, 2024 • edited Loading

irm-codebase commented Jul 18, 2024 • edited Loading

irm-codebase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brynpickering left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2024 • edited Loading

Codecov Report

brynpickering commented Aug 2, 2024

irm-codebase commented Aug 2, 2024

irm-codebase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irm-codebase Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irm-codebase commented Aug 24, 2024

irm-codebase commented Jul 16, 2024 •

edited

Loading

Removal of double `math` objects

irm-codebase commented Jul 18, 2024 •

edited

Loading

irm-codebase commented Jul 18, 2024 •

edited

Loading

brynpickering left a comment •

edited

Loading

codecov bot commented Jul 19, 2024 •

edited

Loading

irm-codebase Aug 17, 2024 •

edited

Loading