Closed macflo8 closed 1 month ago
Attention: Patch coverage is 9.71330%
with 3653 lines
in your changes are missing coverage. Please review.
Project coverage is 52.2%. Comparing base (
31950ed
) to head (f214ed0
).
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?
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.
At https://github.com/iiasa/message-ix-models/pull/189#issuecomment-2078748183 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 https://github.com/iiasa/message-ix-models/pull/107#issuecomment-2062609910, 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.
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: isversion_control
really a subtopic ofmaterial
? 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 likegit 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.
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 :)
This should be celebrated and mentioned in the next weekly MESSAGE meeting (at least :)
Yes definitely! 🥳 Many thanks to both of you @glatterf42 @khaeru for the help and guidance with the migration and everything else for this PR!
Very well done, and congrats!
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:
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
PR checklist
- [ ] Continuous integration checks all ✅Not possible due to missing tests[x] Update doc/whatsnew.