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

Allow mapping distinct names for MESSAGE commodity → MACRO sector #719

Merged
merged 30 commits into from
Jul 25, 2023

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Jun 14, 2023

This functionality is implied to work but, per the added test on the initial commits on this branch, does not. See e.g. here:

RuntimeError: 0-price found in MESSAGE variable PRICE_COMMODITY for commodity "FOO" in node "Westeros".

This occurs because of code like:

model_price.rename(
columns={"lvl": "value", "commodity": "sector"}, inplace=True
)

Data originating from MESSAGE's PRICE_COMMODITY is sent to MACRO's price_MESSAGE, with the commodity dimension renamed to sector, but the mapping provided by the user in the config data is never applied.

This code ultimately dates back to #223, and was not altered in #327. Tests were never added for this functionality.

The PR fixes the bug and makes other improvements:

  • Document each atomic function in the calculations; closes message_ix.macro lacks minimal documentation #315.
  • Use genno to structure the functions, as suggested at
    (This is done through method chaining, the bottom of which is aconst()
    # NB this means it could be rewritten using reporting)
  • Allow the user to omit e.g. price_ref. In this case, as is common in actual use, they are back-extrapolated from data (e.g. PRICE_COMMODITY) in the scenario which is being calibrated.
  • Accept either path to a file with calibration data, or a dictionary, allowing some of the data to be constructed as needed without writing to file; see e.g. Miscellaneous enhancements for 2023-W24 message-ix-models#104.
  • Correct typos in the MACRO GAMS code.

Housekeeping:

  • Activate ixmp.utils.sphinx_linkcode_github; items in the documentation are now linked directly to their source on GitHub.

How to review

  • Build and view the updated docs.
  • Skim the updated test code.
  • (Optional) Run existing downstream/user code which uses Scenario.add_macro; confirm that it continues to work as expected.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
    • Update and re-enable the multiregion tests.
  • Add, expand, or update documentation.
  • Update release notes.

@khaeru khaeru added the bug Doesn't work as advertised/unintended effects label Jun 14, 2023
@khaeru khaeru self-assigned this Jun 14, 2023
@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #719 (de613c8) into main (cec4092) will increase coverage by 0.0%.
The diff coverage is 97.6%.

@@          Coverage Diff          @@
##            main    #719   +/-   ##
=====================================
  Coverage   94.4%   94.4%           
=====================================
  Files         43      43           
  Lines       3448    3434   -14     
=====================================
- Hits        3257    3245   -12     
+ Misses       191     189    -2     
Impacted Files Coverage Δ
message_ix/reporting/computations.py 100.0% <ø> (ø)
message_ix/macro.py 96.6% <96.3%> (+0.1%) ⬆️
message_ix/core.py 95.1% <100.0%> (+<0.1%) ⬆️
message_ix/tests/test_macro.py 100.0% <100.0%> (+0.5%) ⬆️

khaeru added a commit that referenced this pull request Jun 15, 2023
@khaeru khaeru added the macro MACRO model or MESSAGE-MACRO link label Jul 12, 2023
- Add _notna() reference value.
- Expand typing.
- Remove redundant/trivial methods k0()
- Explictly retrieve vars in calibrate()
@khaeru khaeru mentioned this pull request Jul 24, 2023
1 task
- Also ignore missing typing in scipy.optimize.
- Work around khaeru/genno#54 (Quantity.pipe).
@khaeru khaeru added this to the 3.8 milestone Jul 24, 2023
@khaeru khaeru requested a review from a team July 24, 2023 15:13
pytest.raises(ValueError, c.read_data)

c = macro.prepare_computer(westeros_solved, data=data)
with pytest.raises(Exception, match="Missing expected columns.*year"):
Copy link
Member

Choose a reason for hiding this comment

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

The formulation of match seems odd to me here, so just to make sure: is the raised error message really referencing something like columns.gdp_calibrate.year? The MissingKeyError in genno seems to be phrased differently, at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, yes. This exception is raised by the function .macro._validate_data()—in full it is something like "Missing expected columns for gdp_calibration: {'year'}". pytest.raises(…, match=…) is treated as a regular expression, so the .* matches a bunch of characters in the middle.

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.

Apart from that one clarifying question, the new tests look good to me, very streamlined.

@glatterf42
Copy link
Member

glatterf42 commented Jul 25, 2023

To build the docs locally, I ran

sphinx-build -T -E -d _build/doctrees-readthedocs -D language=en . _build/html

as suggested in our own docs. This command produces a few warnings, which might be related to your changes:

Cannot locate code for 'message_ix.models.DEFAULT_CPLEX_OPTIONS' or parent class/module             
Cannot locate code for 'message_ix.models.MESSAGE_ITEMS' or parent class/module
Cannot locate code for 'message_ix.macro.MACRO_ITEMS' or parent class/module

The local docs render slightly differently on e.g. https://docs.messageix.org/en/latest/api.html, where the initial list does not have any bullet points in my local version. The same is true for https://docs.messageix.org/en/latest/macro.html, where the nested structure is also lost for the table-of-content-list. But this does not seem to be specific to this PR because it happens on all other pages as well. Apart from that, the docs-part is looking good to me.

@glatterf42 glatterf42 removed the request for review from a team July 25, 2023 13:40
@khaeru
Copy link
Member Author

khaeru commented Jul 25, 2023

This command produces a few warnings, which might be related to your changes

Yes, that's related to the Sphinx extension ixmp.utils.sphinx_linkcode_github. In general that extension tries to identify the specific lines of a file on which an object (class, function, etc.) is defined, and link directly to them. But it fails for module-level constants, because Python's internals do not provide that information or it's harder to get at. See here.

@khaeru khaeru merged commit 4034d8c into main Jul 25, 2023
20 checks passed
@glatterf42 glatterf42 deleted the enh/macro branch November 2, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Doesn't work as advertised/unintended effects macro MACRO model or MESSAGE-MACRO link
Projects
None yet
Development

Successfully merging this pull request may close these issues.

message_ix.macro lacks minimal documentation
2 participants