Clean up Materials migration branch

[macflo8]

This separate branch includes commits to tidy and make MESSAGEix-Materials run on message-ix-models.

How to review

1. Check the removed files and changed code pieces
2. Build-Solve-Report MESSAGEix-Materials

PR checklist

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

• - Add or expand tests; coverage checks both ✅ See Clean up Materials migration branch #189 (comment); will be handled via Test and improve .model.material #194
• Add, expand, or update documentation.
• Update doc/whatsnew.

[khaeru]

@macflo8, this and #188 look good, thank you!

One thing I notice is that the commit messages do not conform to our code style (see the first bullet).

For this -tidy branch with a few commits, it is easy to git rebase -i and use the r/reword command to adjust.

[glatterf42]

@macflo8, I've migrated the required functions now and cleaned up several imports. However, there's still work left to be done: the code quality check (as you analyzed) should be the first, but you also have a doc.rst file in message_ix_models/model/material. These files should generally live under doc/, in this case probably doc/material (since we also have doc/water. This file in particular is calling automodule for several message_data.model.material functions. These should all be migrated now, so please adapt to the new locations.

[codecov]

Codecov Report

Attention: Patch coverage is 11.36108% with 1576 lines in your changes missing coverage. Please review.

Project coverage is 52.2%. Comparing base (9d9fa6c) to head (cc2e59f).

Files	Patch %	Lines
...x_models/util/compat/message_data/calibrate_vre.py	5.5%	340 Missing ⚠️
.../compat/message_data/change_technology_lifetime.py	2.5%	268 Missing ⚠️
...ge_data/manual_updates_ENGAGE_SSP2_v417_to_v418.py	8.0%	195 Missing ⚠️
...l/compat/message_data/calibrate_UE_gr_to_demand.py	9.6%	94 Missing ⚠️
message_ix_models/model/material/data_aluminum.py	12.7%	89 Missing ⚠️
message_ix_models/model/material/data_steel.py	8.9%	81 Missing ⚠️
message_ix_models/model/material/data_methanol.py	0.0%	76 Missing ⚠️
...pat/message_data/calibrate_UE_share_constraints.py	10.8%	74 Missing ⚠️
...sage_ix_models/model/material/data_power_sector.py	5.4%	70 Missing ⚠️
...at/message_data/check_scenario_fix_and_inv_cost.py	6.2%	60 Missing ⚠️
... and 12 more

Additional details and impacted files

@@                     Coverage Diff                      @@
##           migrate-materials_2024-W17    #189     +/-   ##
============================================================
- Coverage                        54.8%   52.2%   -2.7%     
============================================================
  Files                             128     141     +13     
  Lines                           10011   11342   +1331     
============================================================
+ Hits                             5491    5921    +430     
- Misses                           4520    5421    +901     

Files	Coverage Δ
message_ix_models/cli.py	93.4% <ø> (ø)
message_ix_models/model/material/build.py	17.1% <100.0%> (+17.1%)	⬆️
...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% <ø> (ø)
message_ix_models/model/material/data_generic.py	15.1% <57.1%> (+15.1%)	⬆️
message_ix_models/model/material/data_cement.py	10.9% <55.5%> (+10.9%)	⬆️
message_ix_models/model/material/data_buildings.py	0.0% <0.0%> (ø)
...l/material/material_demand/material_demand_calc.py	18.8% <45.4%> (ø)
... and 18 more

... and 2 files with indirect coverage changes

[macflo8]

Thanks, @glatterf42 for taking care of that 🙏🏻 I will have a look at the materials docs.

[macflo8]

The materials doc file is now moved and the code quality checks are passing. Do you agree to merge this PR @khaeru @glatterf42 ?

[khaeru]

Looking at step (9) in our migration guide:

9. Merge the clean-up branch from (7) into (6), and then (6) into main.

… I see the following things:

• The base PR Migrate MESSAGEix-Materials updates #188 is still in a draft state.

• The base PR checklist has unchecked items.

• The base PR shows the message “This branch is out-of-date with the base branch” —it should be updated by rebasing migrate-materials_2024-W17 on to main, and then rebasing this migrate-materials_2024-W17-tidy on to migrate-materials_2024-W17.

• The current PR shows "This branch cannot be rebased due to conflicts". The above step might make this go away.

• The current PR checklist has unchecked items, and some items are missing without comment.

• Per my earlier comment about commit messages, there are still some like:

  • “resolve conflict 2”
  • “resolve rebase conflict”
  • “resolve rebase conflict 3”

  (Many others still use the passive voice, like “Sets added”, but I guess this could be tolerated.)

Once we address these items, then we can make a simultaneous approval of both PRs, and then perform step (9).

[macflo8]

Thanks @khaeru!

• The base PR is now up to date with main
• The clean-up PR is also rebased
• I adjusted the PR checklist as discussed earlier today. I was not sure about the "Update doc/whatsnew". Should we update this?
• The unused files mentioned in @glatterf42's comment Migrate MESSAGEix-Materials updates #188 (comment) are now removed
• The mentioned commit messages have been rewritten to comply with the standard

[khaeru]

• I was not sure about the "Update doc/whatsnew". Should we update this?

Please have a look at these past commits to the file, particularly this one and this one. Something similar and short should be fine.

Please add the commit to this branch; then it will automatically be included for #188 as well.

[glatterf42]

Thanks, this is looking good now. Please open a new issue to mention and track that the materials team will provide tests at a later date (ideally within a few weeks or months).

@khaeru, would we want to exclude the new files from coverage (to keep the test coverage apparently high) or are we fine with the reduction in coverage (as we were for the nexus module)?

Either way, I think we can then approve this PR and merging this branch to the first migration branch hopefully resolves the code quality issues there, too.

[khaeru]

@khaeru, would we want to exclude the new files from coverage (to keep the test coverage apparently high) or are we fine with the reduction in coverage (as we were for the nexus module)?

I think we ought to be honest, so better to accept the reduction.

[macflo8]

The tracking issue for tests and other pending items is now linked in the PR checklist. I will merge the PR once you approve @glatterf42 @khaeru.

[khaeru]

Please go ahead!

We should use the "merge commit" strategy for both PRs (i.e. not squash-and-merge or rebase-and-merge).

Once this one is merged, we will see the CI on #188 re-run and should check for the same outcome we currently see here.