Skip to content
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

Merged
merged 16 commits into from
Nov 29, 2024
Merged

Enforce consistency in objective dimensions #584

merged 16 commits into from
Nov 29, 2024

Conversation

tsmbland
Copy link
Collaborator

@tsmbland tsmbland commented Nov 28, 2024

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 an asset 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:

costs = timeslice_max(costs)

So ultimately by the time the objectives reach the solver the dimensions are always the same: asset and replacement.

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 and lp_constraint and lp_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 the demand_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.

Capacity_Original_MultiObjective

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

Capacity_New_Capital

Fixes #581

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass: $ python -m pytest
  • The documentation builds and looks OK: $ python -m sphinx -b html docs docs/build

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

tsmbland and others added 13 commits November 28, 2024 18:13
* [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.
@tsmbland tsmbland changed the title Dimensions2 Enforce consistency in objective dimensions Nov 28, 2024
@tsmbland tsmbland marked this pull request as ready for review November 28, 2024 19:03
@tsmbland tsmbland requested a review from dalonsoa November 28, 2024 19:07
Copy link
Collaborator

@dalonsoa dalonsoa left a 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.

@tsmbland tsmbland enabled auto-merge (squash) November 29, 2024 12:12
@tsmbland tsmbland disabled auto-merge November 29, 2024 12:41
@tsmbland tsmbland merged commit e448b09 into v1.3 Nov 29, 2024
14 checks passed
@tsmbland tsmbland deleted the dimensions2 branch November 29, 2024 12:41
@tsmbland
Copy link
Collaborator Author

@dalonsoa One thing I'm a bit unsure about is when it's better to use assert statements and when it's better to explicitly raise an error, like I'm doing here. There are lots of places in the code using assert to check dimensions, which seems like it's effectively the same, but is there an important distinction?

@dalonsoa
Copy link
Collaborator

dalonsoa commented Dec 2, 2024

There's nothing wrong with assert, but raising an error let you control better the type of error (so it is more informative to the user).

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.

@tsmbland tsmbland mentioned this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants