-
Notifications
You must be signed in to change notification settings - Fork 8
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
Enforce consistency in objective dimensions #584
Conversation
* [pre-commit.ci] pre-commit autoupdate updates: - [github.com/astral-sh/ruff-pre-commit: v0.7.4 → v0.8.0](astral-sh/ruff-pre-commit@v0.7.4...v0.8.0) - [github.com/igorshubovych/markdownlint-cli: v0.42.0 → v0.43.0](igorshubovych/markdownlint-cli@v0.42.0...v0.43.0) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This reverts commit 3d223e6.
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.
LGTM! It seems like a good runtime verification with minimal overhead.
@dalonsoa One thing I'm a bit unsure about is when it's better to use |
There's nothing wrong with It also happens that assertions are ignored when running python in optimised mode (with the -O and -OO flags). I've never known anyone using these flags for anything, so I'm not sure if it is a big deal, but you should not trust the behaviour of your code to a feature that might be disabled by the users. In summary, assert should be used only in tests or for debugging purposes. |
Description
See #581 for a description of the problem.
This PR enforces that all objectives must have an
asset
dimension. I've added these checks in the decorator, and modified the "comfort", "efficiency", "capital_costs" and "ALCOE" objectives accordingly, adding anasset
dimension by broadcasting. The tests also needed updating.Now, all objectives are consistent in the dimensions they return, apart from some of them which also return a
timeslice
dimension. However, this ends up being compressed by this line:MUSE_OS/src/muse/investments.py
Line 289 in f94ee1b
So ultimately by the time the objectives reach the solver the dimensions are always the same:
asset
andreplacement
.This seems to fix the problems described in the issue (see case study below), although I haven't really figured out why. I think it's to do with the scipy adapter (
_unified_dataset
andlp_constraint
andlp_constraint_matrix
functions), although the code is so unreadable it's hard to figure out what's going on. I'm not sure it matters though for now, as long as the issue is fixed.To help enforce dimensions, I've created the
check_dimensions
helper function. This is checking both the inputs and outputs of the objective functions. I've also used this in thedemand_share
module, and there are probably many other places it could be used, but I'll leave it like this for now.Case study
I've taken the default model and changed the objective to "capital_costs". I've also increased the capital costs of
gasboiler
10x, so the expected outcome is clear - it should invest in heatpumps in the residential sector.Scenario 1: original code, single objective (capital costs)
The solver fails to find a solution. This is strange as changing the objective is only supposed to change the costs vector, which shouldn't effect whether or not a solution is possible (this is all determined by the constraints)
Scenario 2: original code, adding a dummy second objective (LCOE, 0.01 weight)
Now the model runs to completion and the results are in line with expectations.
I think this works because when the objectives are combined, the
asset
dimension from LCOE gets picked up in the overall combined objective.Scenario 3: new code, single objective (capital costs)
Now we get the expected result, even without adding a dummy objective
Fixes #581
Type of change
Key checklist
$ python -m pytest
$ python -m sphinx -b html docs docs/build
Further checks