-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fix issue with materials and cooling CCS technologies becoming cheaper than non-CCS counterpart #258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are small, looking good. However, I'm wondering whether we could bake requirements such as this into our test suite somehow. This would avoid being reliant on accidental discovery.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
=======================================
- Coverage 77.5% 76.7% -0.9%
=======================================
Files 211 211
Lines 16069 16079 +10
=======================================
- Hits 12469 12333 -136
- Misses 3600 3746 +146
|
Wrote a small script to test with the materials wrapper that calls @glatterf42 Something similar could be integrated into the test suite for all CCS technologies maybe. import ixmp
import message_ix
from message_ix_models.model.material.data_util import gen_te_projections
mp = ixmp.Platform("<platform-name>")
scen = message_ix.Scenario(mp, f"<scenario-name>", "<model-name>")
# SSP1-5
for i in range(1, 6):
inv, fix = gen_te_projections(scen, f"SSP{i}", method="gdp")
for tec in [i + "_NH3" for i in ["biomass", "coal", "gas", "fueloil"]]:
non_ccs = (
inv[inv["technology"] == tec]
.drop(columns=["unit", "technology"])
.set_index(["node_loc", "year_vtg"])
)
ccs = (
inv[inv["technology"] == tec + "_ccs"]
.drop(columns=["unit", "technology"])
.set_index(["node_loc", "year_vtg"])
)
assert ccs.sub(non_ccs).le(0).all().all() == False
# LED scenario
inv, fix = gen_te_projections(scen, "LED", method="gdp")
for tec in [i + "_NH3" for i in ["biomass", "coal", "gas", "fueloil"]]:
non_ccs = (
inv[inv["technology"] == tec]
.drop(columns=["unit", "technology"])
.set_index(["node_loc", "year_vtg"])
)
ccs = (
inv[inv["technology"] == tec + "_ccs"]
.drop(columns=["unit", "technology"])
.set_index(["node_loc", "year_vtg"])
)
assert ccs.sub(non_ccs).le(0).all().all() == False |
@measrainsey —the doc/whatsnew conflict arises from the changes in #225 that I mentioned in Slack. If you rebase you can resolve the conflict by putting your added line from this PR into this section: message-ix-models/doc/whatsnew.rst Lines 54 to 65 in c5e275c
Please ping me if that's not clear. |
Now both CCS and non-CCS variants are mapped to MESSAGE technology `igcc`
ff4c2ef
to
2d1e11d
Compare
Thanks @khaeru ! I rebased and added my edit to doc/whatsnew in the new section -- hope that looks okay! Regarding merging, I quickly chatted with @glatterf42 and we discussed me trying to see if I can add tests similar to the code @macflo8 showed above. I will spend a little time tomorrow to see if I can (or should) add this test -- will report back if I think this can be saved as a later PR or if I want to add it onto this PR! |
d9455f7
to
f680ed1
Compare
hi all, i've added a short test based on @macflo8's code that checks costs for CCS technologies vs their non-CCS variants. and perhaps good thing too because turns out the test failed for the cooling module 😅 so i've also pushed a commit to change the mapping of the CCS-based cooling technologies (i can check with @adrivinca if this is okay and change the cooling technologies' inputs later on if necessary but for now don't consider this a blocker to pass this PR) if this new test looks good to all, then please consider all done and ready on my end, and if we can move forward with merging that'd be great -- thanks!
|
all tests are passing now so if everything looks good, would be great if we can move forward with merging 🙏🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, thanks to you both for coming up with this test and fixing some previously unnoticed bug! That's exactly why @khaeru and I insist on tests as much as we do :)
This appears all checked, approved, and green, so I'll merge. |
Very small PR to change how NH3 CCS technologies in materials module are mapped
@ywpratama noticed during ScenarioMIP review process that costs for NH3 CCS technologies were becoming cheaper than the non-CCS NH3 technologies. This issue happened before in #186, due to CCS technologies being mapped to a different WEO technology than its non-CCS counterpart, as well as having different cost reduction assumptions. At the time, only energy technologies existed within the costs tool, and this new issue pertains to materials/industry technologies.
In this case, because the NH3 CCS technologies were mapped to MESSAGE technology
igcc_ccs
and the non-CCS counterparts were mapped toigcc
, this led to issues of their costs crossing. As a fix, now the NH3 CCS and non-CCS technologies are mapped to MESSAGE energy technologyigcc
.Because this issue is not ScenaripMIP-specific, I think this small fix should best be treated as a standalone PR, merged to main. Then, additional ScenarioMIP-specific changes related to the SSP cost reduction narratives for these technologies can be added in a separate branch (using the functionality added in #255).
How to review
@ywpratama and/or @macflo8: please check if issue of costs for NH3 CCS still persists
For @khaeru and/or @glatterf42 : Read the diff and note that the CI checks all pass.
PR checklist
Add, expand, or update documentation.