-
Notifications
You must be signed in to change notification settings - Fork 159
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
Adjust calculation of PRICE_EMISSION #726
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@glatterf42 thanks for following up on this in today's meeting. I went through the test and actually the tests are not using Westeros tutorials but building new models for testing this. I started to test the changes in this PR using Westeros tutorials. I followed the logic below:
I ran this setup for two types of emission bounds, cumulative and yearly, using Westeros tutorials. My observation is that:
So, my first reaction is that:
Maybe @OFR-IIASA and @gidden can help here. |
Hey @behnam-zakeri - thanks so much for looking into this so quickly. I have a comment and two questions:
|
Dear Behnam, As there were existing test, the idea was to leave those in place and extend the test examples covered, while also making sure that the results are the SAME as opposed to just "close". The test for "duality" was "close" before, but the allowed tolerance was too large, hence the error in the formulation wasnt caught. As you rightly point out, there are a few different cases to test, but I have made a few more adjustments below. TEST 1.: Equal length time periods // CUMULATIVE bound TEST 2.: Unequal length time periods // CUMULATIVE bound TEST 3.: Equal length time periods // PERIOD bound TEST 4.: Unequal length time periods // PERIOD bound TEST 5.: Test price duality
-> Scenario
In the end, we need to ensure that the carbon-price from a cumulative constraint, provides the same emission trajectory per period as the cumulative trajectory when applied as |
@gidden thanks for the reflection. |
@OFR-IIASA thanks for the updates and confirming the logic for the tests. I see your point, if we need varying model periods we need to develop a new test model. |
41c5dd1
to
99f6866
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@glatterf42 I made the following changes and now this PR is ready for reviewing/improving the tests. Indeed, the existing tests are good enough, but a few of them, i.e., The changes made today are:
|
Still working on getting test case 5 to work. I've tried using This might speak to a problem in the conversion between emission bounds and emission prices. If anyone has any idea how to troubleshoot this further or even what specifically might cause this problem, I'm happy to take suggestions :) |
This comment was marked as outdated.
This comment was marked as outdated.
Some reflects to the previous comment from @glatterf42. We know Then how we would know this is a "correct" PRICE_EMISSION? My understanding of why we are doing this duality test (the set " Larger-size scenarios now reach such duality while small-size scenarios are still difficult to reproduce, after another several tries (i.e., adding many technologies (up to 9), adding multiple demands and multiple technologies serving the output of more than one demand, including historical emissions). We can try an open-source snapshot, putting the following before the tests you have already created.
Title: MESSAGEix-GLOBIOM 1.1 R11 no-policy baseline Though, as you sensed, the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #726 +/- ##
=====================================
Coverage 24.0% 24.0%
=====================================
Files 48 48
Lines 4394 4394
=====================================
Hits 1057 1057
Misses 3337 3337
|
This PR revises the calculation of the
PRICE_EMISSION
to correctly account for the period-duration when deriving the period-specificPRICE_EMISSION
from a single marginal value which covers multiple periods (cumulative constraint).How to review
developer (someone like the reviewer) will be able to understand what the code
does in the future.
PR checklist
Note
I'm editing this description to keep track of what has happened in this PR and what's still required. My edits are restricted to below this point.
Notes on progress
On January 22, 2025, @glatterf42 went through the posts here and marked everything as outdated that's no longer relevant to finishing this PR. Several things are still outstanding:
PRICE_EMISSION
is calculated and needs to be updated in these cases.This last point is really the main issue here and the focus of (almost) all posts not marked as outdated: what kind of test do we want to have for the
PRICE_EMISSION
feature and and how we we write it so that it passes?@OFR-IIASA outlined five test cases that would be sufficient and four of them are already present and passing. However, the fifth case is still missing, even after several tries by @behnam-zakeri, @yiyi1991, and @glatterf42. The jupyter notebook in this PR was just used for testing and debugging this work, and should be dropped before merging this PR to
main
.Thanks to discussions with the MESSAGE team, we believe that the issue lies in the complexity of the scenarios used for testing. When using the
PRICE_EMISSION
fix at hand with full global models, both @OFR-IIASA and @yiyi1991 confirmed that the fifth test case is passing fine. Thus, our assumption is that the test is not passing here because the scenario used is too simple: for example, with an insufficient number of technologies defined, the model might decide to use the cheap, high emission technology as long as it can before switching suddenly to the expensive, low emission technology. It seems that such pointwise behavior (as opposed to a shift on a continuum of technologies) causes the duality we want to test for to no longer hold.One possible workaround is to use a global model for testing -- and we already do that: our nightly tests load the full global snapshot from zenodo. Since this is a large model, that takes a significant amount of time, about 45 minutes (estimated by @yiyi1991). Thus, we don't want to include this in our regular CI suite. We could instead think about migrating this test to the nightly tests, where more runtime is not all that problematic.
Alternatively, we could also invest some time to develop/fine-tune a scenario just for this missing duality test.