Skip to content

Conversation

@junukitashepard
Copy link

@junukitashepard junukitashepard commented Oct 13, 2025

Overview

The merged code allows users to represent trade bilaterally for specified commodities. By default, the tool bilateralizes existing fuel trade (e.g., oil_exp).

The bilateralization tool, bilateralize, is generalized for any traded commodity, whether that is a fuel (e.g., LNG), or a material (e.g., steel). It also explicitly represents bilateral trade “flows”, or how a fuel/commodity is transported from exporter to importer. These flow technologies are user defined and flexible; the most common are pipelines (e.g., gas pipelines), maritime shipping (e.g., LNG tanker), and transmission lines.

See https://github.com/iiasa/message-ix-models/blob/project/newpathways-trade/doc/api/tools-bilateralize.rst for more details on implementation.

How to review

Note: All scripts are additions (no existing scripts are amended). Therefore, I would request reviewing code directly rather than individual commits (of which there are >200).

These review steps are also outlined in https://github.com/iiasa/message-ix-models/blob/project/newpathways-trade/message_ix_models/tools/bilateralize/workflow.py

  1. The function prepare_edit_files() should produce untracked files in commodity-specific directories in data/bilateralize/edit_files and also transfer required parameter files to data/bilateralize/bare_files
  2. The function bare_to_scenario() should produce a dictionary of required parameter updates for bilateralization for all specified trade commodities in config.yaml.
  3. The function load_and_solve() should be able to be used to update SSP base scenarios
  4. The documentation should make sense even though it is incomplete. It is currently conceptual and still needs function documentation.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • Update doc/whatsnew.

@junukitashepard junukitashepard changed the title Merge bilateralization tool (project/newpathways-trade) Add bilateralization tool (project/newpathways-trade) Oct 14, 2025
@junukitashepard junukitashepard self-assigned this Oct 29, 2025
@junukitashepard junukitashepard added enh New features or functionality p:NEWPATHWAYS NEWPATHWAYS project labels Oct 29, 2025
@junukitashepard junukitashepard marked this pull request as ready for review October 29, 2025 19:31
@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 58.19328% with 597 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.7%. Comparing base (2bc42a3) to head (7d21579).

Files with missing lines Patch % Lines
...odels/tools/bilateralize/historical_calibration.py 28.8% 212 Missing ⚠️
message_ix_models/tools/bilateralize/pull_gem.py 18.0% 127 Missing ⚠️
...age_ix_models/tools/bilateralize/load_and_solve.py 46.1% 84 Missing ⚠️
...e_ix_models/tools/bilateralize/bare_to_scenario.py 60.6% 63 Missing ⚠️
..._models/tools/bilateralize/mariteam_calibration.py 10.1% 53 Missing ⚠️
...ssage_ix_models/tools/bilateralize/prepare_edit.py 89.1% 45 Missing ⚠️
message_ix_models/tools/bilateralize/workflow.py 0.0% 8 Missing ⚠️
message_ix_models/tools/bilateralize/utils.py 90.3% 3 Missing ⚠️
...ix_models/tools/bilateralize/calculate_distance.py 95.5% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #438     +/-   ##
=======================================
- Coverage   75.3%   73.7%   -1.7%     
=======================================
  Files        274     286     +12     
  Lines      22492   23920   +1428     
=======================================
+ Hits       16958   17639    +681     
- Misses      5534    6281    +747     
Files with missing lines Coverage Δ
.../tools/bilateralize/test_historical_calibration.py 100.0% <100.0%> (ø)
...x_models/tests/tools/bilateralize/test_pull_gem.py 100.0% <100.0%> (ø)
message_ix_models/tests/tools/test_bilateralize.py 100.0% <100.0%> (ø)
...ix_models/tools/bilateralize/calculate_distance.py 95.5% <95.5%> (ø)
message_ix_models/tools/bilateralize/utils.py 90.3% <90.3%> (ø)
message_ix_models/tools/bilateralize/workflow.py 0.0% <0.0%> (ø)
...ssage_ix_models/tools/bilateralize/prepare_edit.py 89.1% <89.1%> (ø)
..._models/tools/bilateralize/mariteam_calibration.py 10.1% <10.1%> (ø)
...e_ix_models/tools/bilateralize/bare_to_scenario.py 60.6% <60.6%> (ø)
...age_ix_models/tools/bilateralize/load_and_solve.py 46.1% <46.1%> (ø)
... and 2 more

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@khaeru
Copy link
Member

khaeru commented Oct 29, 2025

Thanks for the work here! In order to merge the following are needed, and addressing these things first will help reviewers focus more squarely on the added contents and functionality.

  • The "PR checklist" in the description should be all complete ✅ or struck out with comments; see here. Per the individual items:
    • "Continuous integration checks all ✅" —I see that there are a few, unrelated tests ERRORing on the "pytest" workflow jobs, but I am also seeing the same errors at Transport improvements for 2025-W39 #430. So I'll address those errors in that PR or another one, and then by rebasing this branch.
    • "Add or expand tests; coverage checks both ✅" —I think this is artificially low because the tests functions are incorrectly decorated and thus do not run. See my comment inline.
    • "Add, expand, or update documentation" —this seems to be done, right? If so, the box can be checked. If not (e.g. there is a plan to still add more documentation in the current PR), please describe.
    • "Update doc/whatsnew" —this could be 1 commit adding 1 line to the file that links to the already-written documentation page. See the blame for the file for examples of prior additions, e.g. ones that added new .tools submodules.
  • We prefer a linear history. This can be achieved with git rebase main, which will take all the commits on this branch and rebuild the branch starting from the latest commit on main.
  • Note the bullet on commit messages in the "Code style" docs. To change earlier commit messages to meet the style, there are a few approaches, mainly involving git rebase -i:
    • Use reword and edit the commit message.
    • Reorder and squash commits together. When doing this, the commit message (including subject line) for the second, third, etc. commits can be discarded or edited into the first. This is also a strategy for getting rid of commits that might make the history confusing to read.

Many members of the team have experience doing each of these steps and can help by checking work, pair programming, or joining in to do the changes directly, so please reach out on Slack or otherwise.

@junukitashepard junukitashepard force-pushed the project/newpathways-trade branch 9 times, most recently from ebeeb3c to 6a19239 Compare October 31, 2025 11:20
@khaeru khaeru added this to the 2025-12 milestone Oct 31, 2025
@junukitashepard junukitashepard force-pushed the project/newpathways-trade branch 7 times, most recently from 43eb325 to a62b990 Compare October 31, 2025 12:48
junukitashepard and others added 6 commits November 3, 2025 14:01
1) prepare_edit to generate edit files
2) bare_to_scenario to generate dictionary
3) load_and_solve to update scenario and optionally solve
Update formatting to pass mypy/ruff and allow testing
This removes the NEWPATHWAYS project version of documentation (same content)
Load and solve is now tested
Debug to load_and_solve.py (add "_")
Add encoding to prepare_edit.py
@junukitashepard junukitashepard force-pushed the project/newpathways-trade branch from 341cdcb to 6e0b817 Compare November 3, 2025 13:03
@junukitashepard
Copy link
Author

@khaeru Patch coverage is 51%; however, the scripts that are not covered in testing are those that derive input data based on private data sources. I would of course welcome any input on whether/how to increase this coverage.

@yiyi1991
Copy link
Contributor

yiyi1991 commented Nov 3, 2025

Dear Jun, thanks for opening this! I will really try to use it later on steel intermediate products and bring feedback here.

Just very quickly on test coverage. I see if these three main functions are "tested", then basically all small functions there will be covered.

prepare_edit_files(project_name=project_name, config_name=config_name)
trade_dict = bare_to_scenario(project_name=project_name, config_name=config_name)
load_and_solve(
    project_name=project_name,
    config_name=config_name,
    trade_dict=trade_dict,
    solve=True,
)

Then it will be useful to separate the scenario identifier from the config list and allow a Scenario object to be passed to the key functions (Scenario as their args). It is then also easier to build a test, e.g., making a minimum scenario object, passing it to the function load_and_solve(Scenario, project_name, config_name, trade_dict, solve=True), then one does not need to import all small functions in load_and_solve.py.

This also makes it easier for other colleagues to call this function in other workflows.

@junukitashepard
Copy link
Author

Dear Jun, thanks for opening this! I will really try to use it later on steel intermediate products and bring feedback here.

Just very quickly on test coverage. I see if these three main functions are "tested", then basically all small functions there will be covered.

prepare_edit_files(project_name=project_name, config_name=config_name)
trade_dict = bare_to_scenario(project_name=project_name, config_name=config_name)
load_and_solve(
    project_name=project_name,
    config_name=config_name,
    trade_dict=trade_dict,
    solve=True,
)

Then it will be useful to separate the scenario identifier from the config list and allow a Scenario object to be passed to the key functions (Scenario as their args). It is then also easier to build a test, e.g., making a minimum scenario object, passing it to the function load_and_solve(Scenario, project_name, config_name, trade_dict, solve=True), then one does not need to import all small functions in load_and_solve.py.

This also makes it easier for other colleagues to call this function in other workflows.

Thanks @yiyi1991 for this. load_and_solve loads a base scenario (identified in the configuration), clones it, and then applies the bilateralization parameters. So this note makes me think that perhaps I should break this further into:

  1. Load a scenario and clone based on the config.yaml (Option 1)
  2. Call a scenario directly, meaning there is no cloning and the bilateralization is done directly (Option 2)
  3. Update with bilateralization tool
  4. Solve the scenario

I can implement this if this makes sense to you

@khaeru
Copy link
Member

khaeru commented Nov 4, 2025

Every Python module should have an __init__.py file, even if it is empty. So please add an empty file message_ix_models/tools/bilateralize/__init__.py.

@yiyi1991
Copy link
Contributor

yiyi1991 commented Nov 4, 2025

  1. Load a scenario and clone based on the config.yaml (Option 1)
  2. Call a scenario directly, meaning there is no cloning and the bilateralization is done directly (Option 2)
  3. Update with bilateralization tool
  4. Solve the scenario

I agree with breaking it into several steps.

I was just trying to emphasize that it is better NOT to process a scenario as an identifier string hidden in the config, but as a scenario object.

When other colleagues use it, they usually have one scenario object in their workflow (your option 2 style, as they should prepare a cloned scenario ready to add bilateralization by themselves), and pass it to your step three function, together with the other necessary inputs to get the bilateralization there.

@junukitashepard
Copy link
Author

junukitashepard commented Nov 4, 2025

TODO: Update data references to IEA:

  • WEB should use tools.iea.web.load_data() with update:
    #: Mapping from (provider, year, time stamp) → set of file name(s) containing data.
    FILES = {
    ("IEA", "2024"): ( # Timestamped 20240725T0830
    "2024-07-25/WBIG1.zip",
    "2024-07-25/WBIG2.zip",
    ),
    ("IEA", "2023"): ("WBIG1.zip", "WBIG2.zip"), # Timestamped 20230726T0014
    ("OECD", "2021"): ("cac5fa90-en.zip",), # Timestamped 20211119T1000
    ("OECD", "2022"): ("372f7e29-en.zip",), # Timestamped 20230406T1000
    ("OECD", "2023"): ("8624f431-en.zip",), # Timestamped 20231012T1000
    }
  • NGD code needs to be added to tools.iea
  • CEPII has its own issue and will continue to refer to P-drive until tool is added Add an ExoDataSource for CEPII-BACI dataset #449

Set scenario to a message_ix.Scenario for direct bialteralization in load_and_clone
Otherwise function uses start model/scenario from config.yaml
There is also option to name specific start model/scenario
@junukitashepard
Copy link
Author

  1. Load a scenario and clone based on the config.yaml (Option 1)
  2. Call a scenario directly, meaning there is no cloning and the bilateralization is done directly (Option 2)
  3. Update with bilateralization tool
  4. Solve the scenario

I agree with breaking it into several steps.

I was just trying to emphasize that it is better NOT to process a scenario as an identifier string hidden in the config, but as a scenario object.

When other colleagues use it, they usually have one scenario object in their workflow (your option 2 style, as they should prepare a cloned scenario ready to add bilateralization by themselves), and pass it to your step three function, together with the other necessary inputs to get the bilateralization there.

I've added these options in the latest commit: 177cb8d

@junukitashepard
Copy link
Author

Every Python module should have an __init__.py file, even if it is empty. So please add an empty file message_ix_models/tools/bilateralize/__init__.py.

Added in e989681

@khaeru
Copy link
Member

khaeru commented Nov 6, 2025

I see that with recent commits the new docs page now appears in the preview build! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enh New features or functionality p:NEWPATHWAYS NEWPATHWAYS project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants