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

Migrate MESSAGEix-Materials updates #188

Merged
merged 776 commits into from
May 24, 2024
Merged

Migrate MESSAGEix-Materials updates #188

merged 776 commits into from
May 24, 2024

Conversation

macflo8
Copy link
Contributor

@macflo8 macflo8 commented Apr 25, 2024

This PR migrates the remaining updates of MESSAGEix-Materials to message-ix-models and is planned to complement #130 and potentially supersede it eventually.

Major model changes

Demand projections for all materials commodities now processed all in one module

Base year demand data is moved from code to separate yaml files (example)

Implementation of SSP differentiated dimensions:

  • Demand projections depend on population, GDP and elasticity assumptions of respective SSP
  • Cost projections can be updated to SSP assumptions utilizing the message-ix-models costs tool

Refactored base year harmonization of industry energy consumption using latest IEA data

Steps run so far

The code has been migrated from the private repository with the procedure described in the documentation.

Further steps

How to review

  • Read the diff and note that the CI checks all pass.
  • Run a specific code snippet or command and check the output.
  • Build the documentation and look at a certain page.
  • Ensure that changes/additions are self-documenting, i.e. that another
    developer (someone like the reviewer) will be able to understand what the code
    does in the future.

PR checklist

- [ ] Continuous integration checks all ✅ Not possible due to missing tests

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 9.71330% with 3653 lines in your changes missing coverage. Please review.

Project coverage is 52.2%. Comparing base (31950ed) to head (f214ed0).
Report is 2441 commits behind head on main.

Files Patch % Lines
message_ix_models/model/material/data_util.py 6.5% 561 Missing ⚠️
message_ix_models/model/material/data_methanol.py 0.0% 349 Missing ⚠️
...x_models/util/compat/message_data/calibrate_vre.py 5.5% 340 Missing ⚠️
message_ix_models/model/material/__init__.py 23.3% 295 Missing ⚠️
.../compat/message_data/change_technology_lifetime.py 2.5% 268 Missing ⚠️
...ssage_ix_models/model/material/data_ammonia_new.py 8.5% 245 Missing ⚠️
...ge_data/manual_updates_ENGAGE_SSP2_v417_to_v418.py 8.0% 195 Missing ⚠️
message_ix_models/model/material/data_petro.py 10.1% 142 Missing ⚠️
message_ix_models/model/material/data_aluminum.py 10.3% 138 Missing ⚠️
...l/material/material_demand/material_demand_calc.py 18.8% 129 Missing ⚠️
... and 14 more

❗ There is a different number of reports uploaded between BASE (31950ed) and HEAD (f214ed0). Click for more details.

HEAD has 133 uploads less than BASE
Flag BASE (31950ed) HEAD (f214ed0)
151 18
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #188      +/-   ##
========================================
- Coverage   76.7%   52.2%   -24.6%     
========================================
  Files        112     141      +29     
  Lines       7154   11342    +4188     
========================================
+ Hits        5491    5921     +430     
- Misses      1663    5421    +3758     
Files Coverage Δ
message_ix_models/cli.py 93.4% <ø> (ø)
...age_ix_models/util/compat/message_data/__init__.py 100.0% <100.0%> (ø)
...s/util/compat/message_data/get_historical_years.py 20.0% <ø> (ø)
...ge_ix_models/util/compat/message_data/get_nodes.py 100.0% <ø> (ø)
...util/compat/message_data/get_optimization_years.py 28.5% <ø> (ø)
...ge_ix_models/util/compat/message_data/utilities.py 25.0% <ø> (ø)
...els/util/compat/message_data/update_h2_blending.py 11.7% <11.7%> (ø)
...til/compat/message_data/update_fix_and_inv_cost.py 13.9% <13.9%> (ø)
message_ix_models/model/material/build.py 17.1% <17.1%> (ø)
message_ix_models/model/material/data_generic.py 15.1% <15.1%> (ø)
... and 20 more

@glatterf42
Copy link
Member

Thanks @macflo8, this will be very useful :)
Two immediate questions, though, about the files included: a lot of them are in message_ix_models/data/material/version_control/ and I'm wondering: is version_control really a subtopic of material? If not, I suspect this directory was created to distinguish files under version control and those without, but all files on GitHub are necessarily under version control, so there would be no need to keep this. Something like git mv message_ix_models/data/material/version_control/* message_ix_models/data/material/ might be enough to move all files up on level.
Second, some files are in .../material/deprecated/, which again makes me wonder: if these files are deprecated, do we really need to keep them around?

@khaeru khaeru mentioned this pull request Apr 26, 2024
2 tasks
@khaeru
Copy link
Member

khaeru commented Apr 26, 2024

Something like git mv message_ix_models/data/material/version_control/* message_ix_models/data/material/ might be enough to move all files up on[e] level.

This could also be accomplished by giving a --path-rename argument to git filter-repo during the filtering process that moves files in that specific way.

@khaeru
Copy link
Member

khaeru commented Apr 26, 2024

At #189 (comment) I mentioned the commit message format in our code style.

For this longer branch, it seems there are a lot more commits affected, so maybe the suggestion of manual changes during git rebase -i is not realistic. Please have a look at #107 (comment), particularly the replacements.txt file used; I think extending this file with an appropriate regex would be an easy way to automatically rewrite e.g. "Feat(materials): make petrochemical elasticity ssp dependent" to "Make petrochemical elasticity SSP dependent".

Second, note that #186 has been merged recently. While it is not necessary to constantly rebase this branch and the one for #189, both should be based on the latest main at the future moment when we eventually actually do the two merges.

@macflo8
Copy link
Contributor Author

macflo8 commented Apr 29, 2024

Thanks @macflo8, this will be very useful :) Two immediate questions, though, about the files included: a lot of them are in message_ix_models/data/material/version_control/ and I'm wondering: is version_control really a subtopic of material? If not, I suspect this directory was created to distinguish files under version control and those without, but all files on GitHub are necessarily under version control, so there would be no need to keep this. Something like git mv message_ix_models/data/material/version_control/* message_ix_models/data/material/ might be enough to move all files up on level. Second, some files are in .../material/deprecated/, which again makes me wonder: if these files are deprecated, do we really need to keep them around?

Thanks for the comments @glatterf42

ad 1) Since we still rely on xlsx files for input data in MESSAGEix-Materials, keeping track of changes in these files is challenging since the changes are not displayed in human readable form. As a workaround I have added a simple CLI command to create a copies of all sheets in our xlsx files in csv format. Since we need to merge changes from some other branches like this one, the csv files can help to identify the changes in the input files. So these files are not required for the MESSAGEix-Materials model and can be deleted before merging to main. This is not a very elegant solution, so if you have ideas for a better solution I am happy to discuss. My preference would be to move away from xlsx files eventually. But this would require a significant effort and a refactoring of input data is not at the top of our priority list at the moment..

ad 2) that is a good catch. Yes, I think they can be deleted. I see that they are also still present on the original migrate branch. I will discuss it with @GamzeUnlu95.

@glatterf42
Copy link
Member

Thanks for the explanation, @macflo8 :)
For the input data file format, I don't have any good solution at the top of my head. I can however tell you that the water and buildings modules use csv files for their input data, as far as I know. So maybe what you're doing is close to what you need to be compatible with them already: convert all excel files to csv once and adapt your code to accept that format rather than xlsx. I understand if this is not a top priority; there's no need to do this work in this PR (it might even be considered scope creep). We can always open clean-up PRs with specific tasks to keep reviews manageable :)

@khaeru khaeru added the material MESSAGEix-Materials variant label May 23, 2024
@khaeru khaeru added the enh New features or functionality label May 23, 2024
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.

This is looking good! I think we can merge it with the understanding that #194 (in particular writing tests) should be addressed soon afterwards.

@macflo8 macflo8 merged commit e76bbd3 into main May 24, 2024
24 of 26 checks passed
@macflo8 macflo8 deleted the migrate-materials_2024-W17 branch May 24, 2024 10:04
@glatterf42
Copy link
Member

This should be celebrated and mentioned in the next weekly MESSAGE meeting (at least :)

@macflo8
Copy link
Contributor Author

macflo8 commented May 24, 2024

Yes definitely! 🥳 Many thanks to both of you @glatterf42 @khaeru for the help and guidance with the migration and everything else for this PR!

@khaeru
Copy link
Member

khaeru commented May 24, 2024

Very well done, and congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enh New features or functionality material MESSAGEix-Materials variant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants