Closed GamzeUnlu95 closed 1 month ago
GamzeUnlu95
commented
3 months ago I understand for the versioning, we can create a separate "What's New" section in doc/material/index.rst and describe the changes that were made there without specifying a model version for materials separately.
glatterf42
commented
3 months ago I understand for the versioning, we can create a separate "What's New" section in doc/material/index.rst and describe the changes that were made there without specifying a model version for materials separately.
You could do that, but I think it's preferable to keep the change notes in one place. So please consider creating a new subsection in doc/whatsnew.rst. This could be a list of indented bullet points as for tools-costs, but it could also be its own subsection of the v2024... heading (see e.g. here for how to do that).
khaeru
commented
3 months ago You could do that, but I think it's preferable to keep the change notes in one place. So please consider creating a new subsection in doc/whatsnew.rst. This could be a list of indented bullet points as for
tools-costs, but it could also be its own subsection of thev2024...heading (see e.g. here for how to do that).
To recap the rationale from this Slack message:
main branch as always-ready-to-be-released, and release frequently (at least, more frequently than the lower-level packages in the stack).So my suggestion was:
khaeru
commented
2 months ago @macflo8 @GamzeUnlu95 per discussion at today's MESSAGE meeting, can I suggest to include (as part of this currently active PR) something like the following on the page doc/material/index.rst —perhaps in the section "Code reference"?
.. note::
See also :pull:`130`/the archived branch `materials-migrate <https://github.com/iiasa/message-ix-models/tree/migrate-materials>`_ for a distinct version of :mod:`.material`.
That earlier PR was superseded by :pull:`188`, but may contain usable code or data related to:
- A
- B
- CThis will help reader know that the branch is available, and decide whether they should look at it.
codecov[bot]
commented
1 month ago Attention: Patch coverage is 86.88172% with 61 lines in your changes missing coverage. Please review.
Project coverage is 75.5%. Comparing base (
1e5d8d9) to head (8f20625). Report is 86 commits behind head on main.
macflo8
commented
1 month ago I have rewritten the initial commit history of this PR to remove some unnecessary files. I have also refactored the build functions a bit to make it testable. I made a make_spec() function, borrowed from the transport module and adjusted for materials, that is used for running and testing the MESSAGEix-Materials build. Almost all changes are within the material directories except for 2 small changes to make the new tests pass.
The version documentation for MESSAGEix-Materials 1.1.0 is in a separate file in doc/material. The style is based on the pandas-dev whatsnew docs.
@khaeru The reference to #130 has also been added.
For some reason that I could not trace, one test is failing for the Ubuntu and Windows Python 3.12 setup. Besides that I think the PR is ready to be merged if @khaeru / @glatterf42 agree.
macflo8
commented
1 month ago @measrainsey and I were able to resolve the issue with the failing test in the Python 3.12 setups. There were some odd entries in the relation set B.yaml that look like line counters. Apparently the file is parsed differently with Python 3.12 if these line counters are there. After removing them (see commit here), all tests pass. I am not sure if these line counters are needed. Maybe @khaeru can confirm?
khaeru
commented
1 month ago @measrainsey and I were able to resolve the issue with the failing test in the Python 3.12 setups. There were some odd entries in the relation set
B.yamlthat look like line counters. Apparently the file is parsed differently with Python 3.12 if these line counters are there. After removing them (see commit here), all tests pass. I am not sure if these line counters are needed. Maybe @khaeru can confirm?
Wow, great catch! I think that is mea culpa—they look like scrollback counters from Tmux on my system. I guess they were originally included when I prepared the contents of those files. They are not needed.
Thanks also for the comments above and work done cleaning up this PR. I am in a meeting all week and will likely be able to review/approve on Monday.
macflo8
commented
1 month ago Thanks for the review and all the suggestions, @glatterf42! I will add a commit that extends the type hints of the material code base. I have adopted your suggestions in the docs and the built html looks good to me now.
I would like to add more tests. However since that requires quite a lot of extra work (see responses to the testing-related suggestions for more details), I would prefer to do that in a follow-up PR. As you say, this PR is already quite big.
Regarding the potential use of GitHub Projects: I think it would be indeed a useful tool, that would improve managing our future developments for MESSAGEix-Materials. But I can just speak for myself as one person that works on MESSAGEix-Materials. We should probably also discuss this with @GamzeUnlu95 and @yiyi1991.
macflo8
commented
1 month ago Thanks a lot for helping with and reviewing this PR @glatterf42 & @khaeru. I have added a few more commits that address the few minor suggestions that were remaining. I will go ahead and merge now.
This PR updates MESSAGEix-Materials to version 1.1.0. The model changes are documented in doc/material/v1.1.0.rst:
The PR also adds tests to the MESSAGEix-Materials build on bare RES. Solving is not possible at the moment and can be added with a follow-up PR.
How to review
The PR is rebased on the most recent version of the main branch. The build, solve, and report functionalities are tested and work without problems. Reviewers can:
PR checklist
data/material/material.tar.gz