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

Resolve issues related to technical_lifetime and historical vintages in Westeros tutorials #815

Merged
merged 15 commits into from
May 23, 2024

Conversation

behnam-zakeri
Copy link
Contributor

@behnam-zakeri behnam-zakeri commented Apr 3, 2024

This PR resolves issues related to Westeros "baseline" tutorial, including #525, #609, #813, and #814.
This PR does the following:

  1. Defines vintage-activity years for each technology based on technical_lifetime of that technology (addressing Fix handling of activity years and life time of technologies in the tutorials #525)
  2. Defines vintage and activity for historical parameters, i.e., adding historical years to relevant parameters (addressing technical_lifetime not defined for historical vintages in Westeros tutorial #609, partially related to this comment in Add commodity flow of capacity variables #451, and partially related to Westeros tutorials inconsistent with changes made to baseline #624)
  3. Corrects the units, makes sure they are consistent, and adds a note (partially addressing Units are not correct and consistent in Westeros tutorials #814).
  4. Adds capacity-related parameters for the grid (resolving Electricity grid has no capacity-related parameters in Westeros tutorials #813). An investment cost of 800 USD/kW and fixed O&M cost of 16 USD/kW/a is added.

How to review

  • Load Westeros baseline under this branch, read through the corrected text, and make sure it's understandable.
  • Solve Westeros baseline under the "main" branch (and name it differently)
  • Solve Westeros baseline under this branch and compare it with the older version. The results should show that the objective function is different, but the activity and capacity of coal and wind power plants must be the same in the model horizon.

PR checklist

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

behnam-zakeri added a commit to behnam-zakeri/message_ix that referenced this pull request Apr 3, 2024
@behnam-zakeri behnam-zakeri marked this pull request as draft April 3, 2024 15:06
@behnam-zakeri behnam-zakeri self-assigned this Apr 3, 2024
@behnam-zakeri behnam-zakeri added the tutorials Westeros and Austrian tutorials label Apr 3, 2024
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.6%. Comparing base (0cc8102) to head (fde7729).
Report is 55 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #815     +/-   ##
=======================================
+ Coverage   95.4%   95.6%   +0.1%     
=======================================
  Files         46      46             
  Lines       4354    4335     -19     
=======================================
- Hits        4156    4145     -11     
+ Misses       198     190      -8     
Files Coverage Δ
message_ix/testing/__init__.py 99.6% <100.0%> (ø)
message_ix/tests/test_feature_price_emission.py 100.0% <100.0%> (ø)
message_ix/tests/test_macro.py 100.0% <ø> (ø)
message_ix/tests/test_report.py 100.0% <100.0%> (ø)
message_ix/tests/test_soft_constraints.py 100.0% <100.0%> (ø)
message_ix/tests/test_tutorials.py 97.6% <ø> (ø)
message_ix/tests/util/test_tutorial.py 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks :)
Aside from the ToDos you collected, I'd add this: Please adapt the objective value in this test line to the new one, which is 167594.640625:

_t("w0", f"{W}_baseline", check=[("solve-objective-value", 173795.09375)]),

This seems to be the only reason the tests are failing now (the one on windows-latest-py3.9 is flaky and has nothing to do with this PR).

@glatterf42 glatterf42 added this to the 3.9 milestone Apr 4, 2024
@khaeru khaeru changed the title # Resolve issues related to technical_lifetime and historical vintages in Westeros tutorials Resolve issues related to technical_lifetime and historical vintages in Westeros tutorials Apr 4, 2024
@behnam-zakeri
Copy link
Contributor Author

Thanks @glatterf42 for the quick review. I hadn't changed the objective functions in those test because we need to add investment cost for the grid (one of the ToDos) which will change the objective function. I discussed with @OFR-IIASA this morning, and we believe it's not needed to update the investment cost of wind and coal power plants (the optional ToDo), because this tutorial is not representing the reality of today, or in fact any period in history, and just wants to showcase how the model works with two technologies with different investment cost. As such, I will leave the investment cost for wind and coal power plants as it is for the time being.

@behnam-zakeri behnam-zakeri marked this pull request as ready for review April 4, 2024 22:43
@behnam-zakeri behnam-zakeri removed the request for review from OFR-IIASA April 23, 2024 10:07
behnam-zakeri added a commit to behnam-zakeri/message_ix that referenced this pull request Apr 23, 2024
Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to backtrack here, but one clarifying question and one change request:

Does this PR really close #525, #609, #813, and #814?

And as mentioned by @khaeru in the issue he linked, these lines are not clear to new users because it's unclear what the keys are in the key_or_data argument. Could you please adjust this so that we use make_df() for this line, too, clarifying the keys?

@glatterf42
Copy link
Member

Highjacking this PR to enable ruff for all tutorial files since I started migrating to make_df and was annoyed by the code not following our code style.

As for the question I raised earlier, I have now concluded from your PR description, @behnam-zakeri, that all issues except #814 are indeed closed by this PR since #814 is the only one where you say "partially addressing".
Assuming this is true, once we get the tests to pass, this PR is good to go :)

@behnam-zakeri
Copy link
Contributor Author

Thanks @glatterf42 for the kind reminder and sorry for the delay. This PR indeed closes the issues #525, #609, #813, and #814. I correct the description to show that this resolves #814 fully and not partially.

@glatterf42 glatterf42 linked an issue May 21, 2024 that may be closed by this pull request
glatterf42 pushed a commit to behnam-zakeri/message_ix that referenced this pull request May 22, 2024
glatterf42 pushed a commit to behnam-zakeri/message_ix that referenced this pull request May 22, 2024
@glatterf42 glatterf42 requested a review from khaeru May 22, 2024 10:09
Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me once CI passes :)

Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements here!

Applying the code style to the tutorials is a good step. In westeros_baseline.ipynb, this apparently result in a cell with code like:

# Create a Reporter object to describe and carry out reporting
# calculations and operations (like plotting) based on `scenario`
from message_ix.report import Reporter

# ← The line that was here

# "prepare_plots" enables several to describe reporting operations, e.g.
# "plot activity", "plot capacity", or "plot prices"
# See message_ix/util/tutorial.py for more information
from message_ix.util.tutorial import prepare_plots

rep = Reporter.from_scenario(scenario)  # ← …is now here

prepare_plots(rep)

This means the initial comment is no longer with the line it was meant to describe. This might have been due to ruff's automatic sorting of imports. More readable would be:

from message_ix.report import Reporter
from message_ix.util.tutorial import prepare_plots

# Create a Reporter object to describe and carry out reporting
# calculations and operations (like plotting) based on `scenario`
rep = Reporter.from_scenario(scenario)

# "prepare_plots" enables several to describe reporting operations, e.g.
# "plot activity", "plot capacity", or "plot prices"
# See message_ix/util/tutorial.py for more information
prepare_plots(rep)

Some other requested changes inline, as well as an important question.

message_ix/tests/test_feature_price_emission.py Outdated Show resolved Hide resolved
message_ix/tests/test_feature_price_emission.py Outdated Show resolved Hide resolved
message_ix/tests/test_feature_price_emission.py Outdated Show resolved Hide resolved
message_ix/tests/test_tutorials.py Show resolved Hide resolved
glatterf42 pushed a commit to behnam-zakeri/message_ix that referenced this pull request May 22, 2024
@glatterf42 glatterf42 merged commit bdf5747 into iiasa:main May 23, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment