message_ix icon indicating copy to clipboard operation
message_ix copied to clipboard

Add commodity flow of capacity variables

Open Jihoon opened this issue 3 years ago • 21 comments

This PR cleans up and supersedes #387.

PR checklist

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

Things to review

  • The formulation and the documentation is explanatory about the new changes that are made.
  • Check if there are any issues that can possibly arise from the new formulation. (any cases where it might not work as expected).
  • Building and solving a scenario with the new formulation without any erorrs (can be the Westeros tutorial).

Expected review time: One hour or one and a half hour

Jihoon avatar Mar 31 '21 14:03 Jihoon

Codecov Report

Merging #451 (6fe208a) into main (6fe208a) will not change coverage. The diff coverage is n/a.

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

@@          Coverage Diff          @@
##            main    #451   +/-   ##
=====================================
  Coverage   93.0%   93.0%           
=====================================
  Files         43      43           
  Lines       3373    3373           
=====================================
  Hits        3137    3137           
  Misses       236     236           

codecov[bot] avatar Apr 01 '21 12:04 codecov[bot]

#452 is modifying the CI setup. Please restart the tests here (manually or via rebase) once that one is merged, hopefully within an hour.

khaeru avatar Apr 02 '21 10:04 khaeru

Postponed to v3.4 because:

  • PR checklist is incomplete.
  • 3 requested reviews not done.
  • Requested changes from 1 review not completed.

khaeru avatar May 24 '21 11:05 khaeru

I've added the WIP tag, since there is still more time/investigation needed to finish up the PR, as discussed in today's MESSAGEix meeting. Due to that and the following

  • PR checklist is incomplete,
  • 3 requested reviews not done,
  • Requested changes from 1 review not completed,

we postpone to v3.5.

LauWien avatar Jan 19 '22 17:01 LauWien

The issue related to the historical vintage 690 of coal_ppl seems to be a result of the missing technical_lifetime parameter of that vintage in the Westeros tutorial (see #609).

The problem of non-zero CAP values of technologies after the end of their lifetime is due to the fact that there is no equation that would require CAP to be zero after the end of the technology lifetime. The equation CAPACITY_MAINTENANCE just requires that CAP in a period is lower than in a previous period. This usually does not lead to problems, but in this extension there might be a value in delaying commodity release to better balance a "scrap-type" commodity equation that is set to equal. Usually, there is no cost penalty and also no other benefits as other parameters have not been defined for periods beyond the lifetime.

To avoid this behavior, we may need to introduce an additional equation of the following kind:

CAPACITY_MAINTENANCE2(node,inv_tec,vintage,year)$( map_tec(node,inv_tec,vintage) AND NOT map_tec_lifetime(node,inv_tec,vintage,year) 
AND NOT first_period(year) AND year_order(vintage) < year_order(year))..
    CAP(node,inv_tec,vintage,year) =L= 0 ;

volker-krey avatar May 23 '22 08:05 volker-krey

The issue related to the historical vintage 690 of coal_ppl seems to be a result of the missing technical_lifetime parameter of that vintage in the Westeros tutorial (see #609).

The problem of non-zero CAP values of technologies after the end of their lifetime is due to the fact that there is no equation that would require CAP to be zero after the end of the technology lifetime. The equation CAPACITY_MAINTENANCE just requires that CAP in a period is lower than in a previous period. This usually does not lead to problems, but in this extension there might be a value in delaying commodity release to better balance a "scrap-type" commodity equation that is set to equal. Usually, there is no cost penalty and also no other benefits as other parameters have not been defined for periods beyond the lifetime.

To avoid this behavior, we may need to introduce an additional equation of the following kind:

CAPACITY_MAINTENANCE2(node,inv_tec,vintage,year)$( map_tec(node,inv_tec,vintage) AND NOT map_tec_lifetime(node,inv_tec,vintage,year) 
AND NOT first_period(year) AND year_order(vintage) < year_order(year))..
    CAP(node,inv_tec,vintage,year) =L= 0 ;

After the addition of this equation, the issues related to CAP remaining after the technology lifetime is solved and scrap is generated correctly in the years that are not the first model period.

GamzeUnlu95 avatar May 24 '22 13:05 GamzeUnlu95

@GamzeUnlu95, the issue was the use of the set year2 which only covers years as of the first model year. Replacing it by year_all2 does the job, I believe.

volker-krey avatar Jun 13 '22 09:06 volker-krey

New changes work fine with the first year. However, when lifetime is not exactly within duration_period (e.g. lifetime 25 and duration period is 10) the formulation is not working as expected since the conditions for map_tec_lifetime set change. In the current conditions for the first year map_tec_lifetime should not include the first year in the set but this is not true for lifetimes out of duration period. The set still includes the first year but a certain percentage of the capacity is retired and therefore scrap is still expected in the first year.

GamzeUnlu95 avatar Jun 13 '22 15:06 GamzeUnlu95

I removed my name from the reviewer list due to notifications. Please let me know when I need to review something.

behnam-zakeri avatar Jun 13 '22 15:06 behnam-zakeri

@khaeru do you have any idea about the failing tests here:

  1. mypy: message_ix/tests/test_feature_vintage_and_active_years.py:29: note: Possible overload . And two more errors in the same line. The branch is recently rebased and this section is the same as the main branch.

  2. Tutorial test which is testing the notebooks wheter they can be run without errors or not is failing (test_tutorials.py line 110): R_austria and R_austria_load_scenario. (Only in Test / windows-latest-py3.10 and Test / windows-latest-py3.7). I am able to run both of these in my own computer with this new branch.

GamzeUnlu95 avatar Jun 22 '22 09:06 GamzeUnlu95

  1. mypy: message_ix/tests/test_feature_vintage_and_active_years.py:29: note: Possible overload . And two more errors in the same line. The branch is recently rebased and this section is the same as the main branch.

This might be due to an updated version of mypy. It sometimes introduces new, more precise type checking, so our existing code that was okay with previous mypy versions starts to fail. I can investigate this one.

  1. Tutorial test which is testing the notebooks wheter they can be run without errors or not is failing (test_tutorials.py line 110): R_austria and R_austria_load_scenario. (Only in Test / windows-latest-py3.10 and Test / windows-latest-py3.7). I am able to run both of these in my own computer with this new branch.

This is what's called a "flaky" test: sometimes, randomly, it times out and fails on the GitHub Actions runners. The response is just to re-run that 1 failed job, which I'll do for you now.

khaeru avatar Jun 24 '22 08:06 khaeru

This might be due to an updated version of mypy.

Checking locally:

  • mypy 0.961, numpy 1.21.5 —passes
  • mypy 0.961, numpy 1.23.0 —shows the same error as the failing lint check, which shows numpy 1.22.x.

So the issue appears to be the newer numpy, not a new mypy. I will make a separate PR, then you can rebase this one.

khaeru avatar Jun 24 '22 08:06 khaeru

I will make a separate PR, then you can rebase this one.

Oliver helpfully reviewed that PR. Rebasing this branch on main should make the lint error disappear.

khaeru avatar Jun 27 '22 08:06 khaeru

Agreed with @GamzeUnlu95 to move this to 3.8.

glatterf42 avatar Apr 20 '23 08:04 glatterf42

@GamzeUnlu95, @Jihoon, @macflo8: what is the status of this PR? Are you currently using it? What needs to be done to finish it off? Do you think it is realistic to do so by the end of this year?

glatterf42 avatar Dec 06 '23 08:12 glatterf42

I'm going to assume that this won't be ready this week, so we will postpone it to the 3.9 milestone. However, I'd be happy to help here, so please let me know what this PR requires to be ready to merge.

glatterf42 avatar Dec 13 '23 09:12 glatterf42

@glatterf42 Sorry for the delayed response. This branch contains additions to the MESSAGEix GAMS code. The additions are run by default and add ~10 minutes to the total runtime per MESSAGE iteration. Merging this PR in its current state would therefore negatively affect the runtime of all MESSAGEix users. Thus, we have to postpone the merge for now. We are planning revisit this PR next year and add a mechanism to run the additional GAMS code not by default but only optionally.

macflo8 avatar Dec 13 '23 09:12 macflo8

Thanks for the update! Unfortunately, I probably won't be able to help writing GAMS code, but moving to the next milestone then seems like the right call :)

glatterf42 avatar Dec 13 '23 10:12 glatterf42

@khaeru suggested (when this PR was discussed in today's monthly meeting), that @daymontas1 recently improved the MACRO code so that regions can solve simultaneously and that similar improvements may be possible here.

glatterf42 avatar Apr 25 '24 12:04 glatterf42

@khaeru suggested (when this PR was discussed in today's monthly meeting), that @daymontas1 recently improved the MACRO code so that regions can solve simultaneously and that similar improvements may be possible here.

To expand a bit on that suggestion:

So among these, there are many opportunities to do things like (a) pass a GAMS parameter that would affect conditional inclusion of other GAMS files (that would be the approach in this PR: unless particular features are switched on, do not include the .gms files that enable that feature but lead to a slow compile) or (b) set a variable or something else that the GAMS code would respond to (this could be the approach in #808 to choose sequential or parallel solve of MACRO).

Where precisely to handle/set defaults/etc. for new options/arguments is easily determined by their scope of use:

  • Any GAMS model usable with ixmp (or ixmp4)-stored data? → ixmp.model.gams.GAMSModel.
  • All of the models shipped with message_ix? → message_ix.models.GAMSModel.
  • Combinations involving MESSAGE, e.g. MESSAGE and MESSAGE_MACRO? → message_ix.models.MESSAGE.
  • Only MACRO? → message_ix.models.MACRO.

I think by following the structure and example of existing code it is fully feasible to integrate this and the other PR.

khaeru avatar Apr 25 '24 13:04 khaeru

@Jihoon @GamzeUnlu95 @macflo8 Is there any update on this PR? Since we probably want to publish a 3.9 release still this week, I'll postpone this to the 3.10 milestone.

glatterf42 avatar May 23 '24 12:05 glatterf42