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

Wat ssp upd4 #268

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Wat ssp upd4 #268

wants to merge 7 commits into from

Conversation

adrivinca
Copy link
Contributor

@adrivinca adrivinca commented Dec 10, 2024

Required: write a single sentence that describes the changes made by this PR.

Fixes for the SSP update.
we noticed nuc_lc cooling technology was not correctly parametrized. I did not notice it because in all the scenarios I tested the technology was not active, but in some scenario it is, before 2020

Oliver suggested to pre-filter the remove.set as currently it takes lot of time just to check if the set is there

How to review

Not, sure changes are quite minimal and I already cherry picked them in ssp_dev

PR checklist

  • Continuous integration checks all ✅
  • whatsnew.

@adrivinca adrivinca added the water MESSAGEix-Nexus (water) variant label Dec 10, 2024
@adrivinca adrivinca self-assigned this Dec 10, 2024
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.7%. Comparing base (44087e4) to head (3f724af).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
message_ix_models/model/water/data/water_supply.py 50.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #268     +/-   ##
=======================================
- Coverage   77.6%   76.7%   -0.9%     
=======================================
  Files        211     211             
  Lines      16079   16102     +23     
=======================================
- Hits       12481   12355    -126     
- Misses      3598    3747    +149     
Files with missing lines Coverage Δ
message_ix_models/model/water/build.py 93.5% <100.0%> (+0.1%) ⬆️
message_ix_models/model/water/data/demands.py 78.8% <100.0%> (+0.2%) ⬆️
...essage_ix_models/model/water/data/water_for_ppl.py 95.4% <100.0%> (+0.2%) ⬆️
message_ix_models/model/water/data/water_supply.py 76.3% <50.0%> (-0.3%) ⬇️

... and 8 files with indirect coverage changes

@adrivinca adrivinca requested a review from glatterf42 December 10, 2024 13:28
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 :)

Copy link
Member

Choose a reason for hiding this comment

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

So this is a for inside a for inside a for? I'm surprised that this is the faster solution, it doesn't seem very efficient. Without spending more time, I'm not sure how to improve this, though, and if you say it's an improvement, then it's fine with me :)

Copy link
Contributor Author

@adrivinca adrivinca Dec 10, 2024

Choose a reason for hiding this comment

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

the thing is that for any item to be removed, the message_ix_models.model.build.apply_spec() checks if elements exist and proceeds or says that there are no elements. This is very slow.
now we just exclude in advance sets from the remove list that we know are not in the scenario, so that they do not get sent to apply_spec()

Copy link
Member

Choose a reason for hiding this comment

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

If this is a serious problem and/or you know a solution for it, please record it in a new issue. This will raise awareness and eventually lead to a solution :)

some calibration scenarios run 2020 and 2025 free, the previous code should in theory no break, but it is safer to look at fully historical years
Move adjustment at the end of the script, for all technologies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
water MESSAGEix-Nexus (water) variant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants