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

Regional emission pools #514

Draft
wants to merge 47 commits into
base: main
Choose a base branch
from
Draft

Conversation

volker-krey
Copy link
Contributor

@volker-krey volker-krey commented Sep 29, 2021

  • added new parameter emission_sink_rate

  • added new equation EMISSION_POOL with variable EMISS_POOL

  • possibility to limit size of pools via constraint currently not included

  • note that I did not run the code, so it has not been tested for correct syntax

  • get initial draft code to work

  • add conditional formulation "is_emission_sink"

  • add "is_emission_sink", "is_bound_emission_pool", "PRICE_EMISSION_POOL" and "EMISS_POOL" to backend.

  • add bound formulation.

  • add post-processing for PRICE_EMISS_POOL

  • add cost formulation.

  • document all new code in GAMS

  • Complete tutorial

  • Setup tests

  • Add test for shifting first model year and updating of the historical values

  • the parameter historical_emission_pool needs to be added as part of the parameters which get updated during the cloning process; add test.

  • discuss how to treat PRICE_EMISS_POOL - currently this only includes shadow price from bound_emission_pool_up, i.e. it represents the cost of mitigating similar to PRICE_EMISS. Should we also include lower bound related costs separately?

@volker-krey volker-krey added the enh New features & functionality label Sep 29, 2021
@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.1%. Comparing base (24934f7) to head (93137ef).
Report is 793 commits behind head on main.

Current head 93137ef differs from pull request most recent head c7016ad

Please upload reports for the commit c7016ad to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #514     +/-   ##
=======================================
- Coverage   93.8%   93.1%   -0.8%     
=======================================
  Files         40      41      +1     
  Lines       3057    3235    +178     
=======================================
+ Hits        2870    3014    +144     
- Misses       187     221     +34     
Files with missing lines Coverage Δ
message_ix/models.py 100.0% <100.0%> (ø)
message_ix/tests/test_emission_pool.py 100.0% <100.0%> (ø)

... and 2 files with indirect coverage changes

@OFR-IIASA
Copy link
Contributor

@volker-krey - I have added a checklist of tasks to be completed. The initial setup now works and the calculations for the tutorial seem correct (see attached calculation in xlsx: Westeros_Calculation_check.xlsx).
@khaeru we need some advice on how to proceed with two specific items.

  1. we have a new set "is_emission_sink". These are normally calculated in the backend as far as I know. Currently, we manually create this in the tutorial, but we should resolve this for the final PR.
  2. we also want to avoid having to pass the var "EMISS_POOL" in the solve command to be able to get the results. This is also I believe done in the backend?

@khaeru
Copy link
Member

khaeru commented Sep 30, 2021

@khaeru we need some advice on how to proceed with two specific items.

Please read this file

MESSAGE_ITEMS = {
# Index sets
"commodity": dict(ix_type="set"),

You are correct that some things like this were originally done in the Java/ixmp_source/JDBCBackend code, but we are trying to move away from that. This is in order to have a cleaner separation between the layers of the stack, which means that ixmp (including any Backend, Java or not) should not have any hard-coded names of sets, variables, etc.; it should be agnostic about them. The message_ix package is the proper place to name and define particular sets, parameters, etc. that are particular to MESSAGE (the word "scheme" is used in some places); MESSAGE_ITEMS is where these are stored.

See for example how this was done in #190: https://github.com/iiasa/message_ix/pull/190/files#diff-62f44bbc1d152875567f99d639addb547727233454fe1958278c45770d6a2912

@OFR-IIASA
Copy link
Contributor

OFR-IIASA commented Sep 30, 2021

@khaeru we need some advice on how to proceed with two specific items.

Please read this file

MESSAGE_ITEMS = {
# Index sets
"commodity": dict(ix_type="set"),

You are correct that some things like this were originally done in the Java/ixmp_source/JDBCBackend code, but we are trying to move away from that. This is in order to have a cleaner separation between the layers of the stack, which means that ixmp (including any Backend, Java or not) should not have any hard-coded names of sets, variables, etc.; it should be agnostic about them. The message_ix package is the proper place to name and define particular sets, parameters, etc. that are particular to MESSAGE (the word "scheme" is used in some places); MESSAGE_ITEMS is where these are stored.

See for example how this was done in #190: https://github.com/iiasa/message_ix/pull/190/files#diff-62f44bbc1d152875567f99d639addb547727233454fe1958278c45770d6a2912

In the PR, we have already updated MESSAGE_ITEMS. The issue as far as I understand though is that "is_*" needs to be derived dynamically when solving based on the parameters in the scenario as this is a helper table to avoid 0 values being misinterpreted by GAMS.

@khaeru
Copy link
Member

khaeru commented Oct 1, 2021

The issue as far as I understand though is that "is_*" needs to be derived dynamically when solving based on the parameters in the scenario as this is a helper table to avoid 0 values being misinterpreted by GAMS.

For these "generated", "derived", or "mapping" sets, there are at least three approaches:

  1. In Java/ixmp_source/JDBCBackend, during the step of writing out the Scenario data to a GDX file. This approach is 🚫 because it conflates (ixmp backend or I/O operation of writing Scenario content to GDX) with (message_ix model formulation and associated logic, i.e. the meaning and significance of that content—something about which ixmp must be agnostic, per previous comment). I mention it here because it is still in use at the moment, but we should be aiming to eliminate this code, and should not create more of it.
  2. In the GAMS code directly. I understand this is already done in some cases. As @behnam-zakeri notes at Zero values in GDX #516, however, sometimes the mapping set is required before the corresponding parameter data, in order to ensure it is read correctly (with GAMS EPS = 0 versus missing values). One advantage here is that people using the GAMS code without the Python API have an easier time, because they don't need to populate the sets manually.
  3. In Python, in message_ix.models.

IMHO (3) is preferable for a few reasons:

  • More developers are able to understand and improve Python than GAMS or Java.
  • It unites or consolidates the MESSAGE "scheme" and consistency checks/logic specific to the scheme in the same module/class.
  • It can (theoretically) be operated without writing to GDX or attempting to solve a Scenario, which means atomic operations are better separated.

@khaeru khaeru changed the title draft of regional emission pool implementation Regional emission pools Nov 3, 2021
@khaeru
Copy link
Member

khaeru commented Nov 3, 2021

@OFR-IIASA @behnam-zakeri I'm happy to jump in here at some point to demo how the code for the dynamic derivation of the is_* sets should look, per my suggestion above. Please let me know when's a good moment.

@OFR-IIASA
Copy link
Contributor

@OFR-IIASA @behnam-zakeri I'm happy to jump in here at some point to demo how the code for the dynamic derivation of the is_* sets should look, per my suggestion above. Please let me know when's a good moment.

@khaeru many thanks for offering this. I think we are at a stage now where your suggestion makes sense to implement. We have completed the updates to the core formulation and are now at the stage where we want to cleanup the PR in preparation for merging.

@khaeru khaeru self-assigned this Nov 9, 2021
@khaeru
Copy link
Member

khaeru commented Nov 9, 2021

I think we are at a stage now where your suggestion makes sense to implement.

Thanks for the heads-up! Added to my queue.

@khaeru
Copy link
Member

khaeru commented Nov 16, 2021

Added to my queue.

Now done.

  • One of your added tests (message_ix/tests/test_emission_pool.py::test_bound_emission_pool) was failing when I checked out the branch (values not as expected), and it is still failing in the same way after my changes. The other tests pass.
  • Please have a look at how I adjusted the tests; I did not touch the notebooks.
  • At some point, the branch should be rebased on latest main, and then you should each do git pull --rebase. I did not do this for you.
  • Some of these changes should eventually go upstream to ixmp, but we can do that later. What I added should be forward-compatible.
  • Total time about 75 minutes.

@OFR-IIASA
Copy link
Contributor

Thanks @khaeru for adding the code.

  • One of your added tests (message_ix/tests/test_emission_pool.py::test_bound_emission_pool) was failing when I checked out the branch (values not as expected), and it is still failing in the same way after my changes. The other tests pass.

I have adjusted the tests accordingly.

  • Please have a look at how I adjusted the tests; I did not touch the notebooks.

Thanks, I have adjusted the tutorial notebook.

  • At some point, the branch should be rebased on latest main, and then you should each do git pull --rebase. I did not do this for you.

I have rebased the branch.

Lastly, I have also added a test to check the removal of the parameter bound_emission_pool. This also checks the removal of the corresponding is_* set. The test currently fails on the is_*, but we will need to correct this otherwise the problem becomes infeasible.


# Not consistent; empty and then re-populate the set
with scenario.transact(f"Enforce consistency of {set_name} and {par_name}"):
scenario.remove_set(set_name, existing)
Copy link
Member

Choose a reason for hiding this comment

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

@OFR-IIASA this line is supposed to entirely clear the set before it is populated by the next line. You could perhaps check whether this is having the intended effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

@khaeru I have added a two tests to see if this feature works correctly.

  • Test 1. adds the parameter bound_emission_pool for 2 periods, solves and checks is_bound_emission_pool.
    The bound is then extended across another period, solved and checks is_bound_emission_pool
  • Test 2. adds the parameter bound_emission_pool for 3 periods, solves and checks is_bound_emission_pool.
    The bound_emission_pool is removed.
    The bound_emission_pool is then re-added, but only for two periods, solved and checks is_bound_emission_pool.
    Both the tests work and confirm the working of the code above.

Copy link
Contributor

@OFR-IIASA OFR-IIASA Dec 13, 2021

Choose a reason for hiding this comment

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

@khaeru The above code has tests, but for the functionality for which the code, in my understanding was designed. The design was to ensure that the "is_*" is consistent when the corresponding parameters is being modified; the code does not cover the removal of the corresponding parameter. The removal is test in a test test_bound_emission_pool_removal(), which fails.
Further, this code is only executed for model="MESSAGE", not when passing the argument model="MESSAGE-MACRO" as part of the solve command.

Copy link
Member

Choose a reason for hiding this comment

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

The design was to ensure that the "is_*" is consistent when the corresponding parameters is being modified; the code does not cover the removal of the corresponding parameter. The removal is test in a test test_bound_emission_pool_removal(), which fails.

Thanks for directing to this specific test. To be clear:

  • The added enforce() method is called at the moment the model is run/solved.
  • In the final lines of that test, there is a call to .remove_par("bound_emission_pool", …), then a call to .commit() but—unlike the other tests—there is no call to .solve() before .set("is_bound_emission_pool") is checked.
  • So enforce() is never invoked, and the result is as expected.
  • I suspect that if you called .solve() before checking the set, then the test would pass.

More detail: a possible choices would be to have enforce() run (a) on every call to .commit(), or even (b) on every call to .add_par(). The current choice is deliberate: it mirrors the behaviour of the Java code underlying JDBCBackend, which only creates these mapping/etc. sets at the moment the data is written out to GDX. That Java code must someday be ported into message_ix. enforce() is now the destination for that code and runs at the same point.

I thought this was better than introducing 1+ additional points at which data is massaged automatically; it would be complex and confusing additional work to think through and plan for possible interactions of these automatic changes. We unfortunately do not have the bandwidth for that at the moment.

Further, this code is only executed for model="MESSAGE", not when passing the argument model="MESSAGE-MACRO" as part of the solve command.

This I can check, and will do after you try adjusting the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I finally got around to sorting out the tests. All tests pass; now the issue would be to ensure that we have this working when the calling solve(model="MESSAGE-MACRO"). Unfortunately I dont think we have a simple test-case which can be adapted and run with macro.

Copy link
Member

Choose a reason for hiding this comment

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

I can try to look at this next week, after we've wrapped up the current release.

volker-krey and others added 24 commits January 26, 2022 09:19
…n documentation

switching order of terms has no impact on results, but is more logical when presenting the material
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants