message_ix icon indicating copy to clipboard operation
message_ix copied to clipboard

Correct calculation of NEW_CAPACITY_CONSTRAINTs

Open OFR-IIASA opened this issue 3 years ago • 5 comments

This PR addresses an issue which occurs when calculating the upper and lower constraints for new capacity (NEW_CAPACITY_CONSTRAINT_LO and NEW_CAPACITY_CONSTRAINT_UP). The issue resulting from the current formulation will vary the magnitude of the bound when the duration_period of the time-period for the bound is being calculated, differs to the previous time-period. An adjustment of the bound is made to take into consideration the change in the duration_period.

How to review

  • Read the diff and note that the CI checks all pass.

PR checklist

  • [x] Continuous integration checks all ✅
  • [x] Add or expand tests; coverage checks both ✅
  • [x] Add, expand, or update documentation.
  • [x] Update release notes.

OFR-IIASA avatar Sep 21 '22 14:09 OFR-IIASA

Codecov Report

Merging #654 (fddee35) into main (27ae6e6) will increase coverage by 93.0%. The diff coverage is n/a.

:exclamation: Current head fddee35 differs from pull request most recent head 3270955. Consider uploading reports for the commit 3270955 to get more accurate results

@@           Coverage Diff           @@
##           main    #654      +/-   ##
=======================================
+ Coverage      0   93.0%   +93.0%     
=======================================
  Files         0      43      +43     
  Lines         0    3378    +3378     
=======================================
+ Hits          0    3142    +3142     
- Misses        0     236     +236     
Impacted Files Coverage Δ
message_ix/message_ix/reporting/computations.py 100.0% <0.0%> (ø)
message_ix/message_ix/core.py 95.0% <0.0%> (ø)
message_ix/message_ix/tests/test_legacy_version.py 100.0% <0.0%> (ø)
message_ix/message_ix/__init__.py 90.0% <0.0%> (ø)
message_ix/message_ix/tests/test_core.py 100.0% <0.0%> (ø)
message_ix/message_ix/tests/test_cli.py 100.0% <0.0%> (ø)
message_ix/message_ix/tests/test_util.py 100.0% <0.0%> (ø)
...age_ix/tests/test_feature_bound_activity_shares.py 100.0% <0.0%> (ø)
...ssage_ix/message_ix/tests/test_soft_constraints.py 100.0% <0.0%> (ø)
message_ix/message_ix/macro.py 96.3% <0.0%> (ø)
... and 33 more

codecov[bot] avatar Sep 21 '22 15:09 codecov[bot]

@behnam-zakeri thanks for the comments; these have been addressed. I am not planning to add tests. As we generally dont test the GAMS-core formulation, doing so for this feature would imply adding tests not only to test this specific change, but then go on to check for example the soft-constraints and further the therewith associated costs for example - just to be complete. This means the tests would become very extensive and this is something that would need to be addressed in a more structured manner.

OFR-IIASA avatar Oct 18 '22 12:10 OFR-IIASA

Hi both - I wonder if it is possible to think of an 'easy' test. For example, we regularly test things like emissions bounds and other aspects.

Is it possible to think of a minimal test where adding a capacity constraint would change the solution?

gidden avatar Oct 18 '22 12:10 gidden

@gidden there is unfortunately no off the shelf example to which to test this change i.e. we have nothing of the shelf to test change in period duration. This would be something interesting to do in one of the tutorials which in turn can be used for tests. The closest we have at the moment is westeros_historical_new_capacity.ipynb. This introduces historical new capacity, but our westeros tutorial doesnt have varying timesteps. For now I have added this as a desired feature to add to the basic westeros tutorial suite, which would serve as a starting point to facilitate these type of tests (see issue #662 ).

OFR-IIASA avatar Oct 18 '22 13:10 OFR-IIASA

@gidden and @behnam-zakeri I have managed to extend the make_westeros() testing utility to accept variants of the model_horizon. This seems to suite our requirements in terms of testing the changes we have made. I also ran these new tests on the current main branch, where we can see that the results are very different and match the manual calculations I did in excel for both test cases. Hope this is sufficient now. Have a look please and see if anything else is missing. Thanks.

OFR-IIASA avatar Oct 19 '22 10:10 OFR-IIASA

Noting now that all tests pass. I will resolve conflicts and merge.

gidden avatar Nov 07 '22 16:11 gidden