-
Notifications
You must be signed in to change notification settings - Fork 50
Water module units + size trimming #371
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
Water module units + size trimming #371
Conversation
adrivinca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleaning work!
I still have to go through tests.
here just some comments for keeping consistency and avoid forgetting about commented parts over time
|
|
||
| land_out["value"] = 1e-3 * land_out["value"] | ||
| land_out["value"] = land_out["value"] | ||
| # FIX-ME : check if input is already in MCM as we are assumptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you have checked it by looking at the GDX, please update this comment and maybe leave a comment with the correct unit, but warning that this might change in different globiom matrix submissions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the gdx has MCM values, could you maybe note it here. so that we keep track at least. thanks
| log.debug(f"cool_tech() for year '{year}'") | ||
| # Identify missing combinations in the current aggregate | ||
| input_cool_2015_set = set( | ||
| # FIXME : This should have been a one off script for debugging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand/remember this. can you please explain? here on Github
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of using a static yaml for the missing techs so we would be able to validate the combinations generated and ensure they make sense. Otherwise, for each run there is a risk of errors propagating
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #371 +/- ##
=======================================
- Coverage 77.0% 76.1% -0.9%
=======================================
Files 246 246
Lines 19819 19866 +47
=======================================
- Hits 15270 15131 -139
- Misses 4549 4735 +186
🚀 New features to boost your workflow:
|
8e6986f to
ccf99cb
Compare
|
@glatterf42 I did a first round of review and I am good with the current changes. We have more PRs coming after this, focusing on
|
ccf99cb to
3bf0b7c
Compare
3bf0b7c to
e00fddd
Compare
5633487 to
1fdacd3
Compare
1fdacd3 to
2efedf3
Compare
|
@glatterf42 I've reworded the commits, and added to whatsnew |
glatterf42
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great. I'd just add an empty line here to enable the rendering as a headline (which I think is the intention).
|
Thanks, there's just one more commit remaining: "Moving pol_scen out of if cond" should just be "Move pol_scen out of if cond". I'll approve the PR already, but please also fix this before merging :) Ah, don't worry about the test case that's currently failing: this has nothing to do with your PR. It's fine to merge by bypassing the rules in this case 😃 |
|
why is the macos test stuck? |
|
See #381 (comment), I've done a bit of digging, but no clear answer yet. Yes, I could merge it if we all agree it's ready :) |
Fixing for ruff in demands.py
- for ScenarioInfo and Scenario inputs.
Update doc/whatsnew.rst Co-authored-by: Fridolin Glatter <[email protected]>
f0402ce to
d93345c
Compare
|
@glatterf42 I think its ready to merge. I did the changes for "Moving Pol scen..." and the update suggestion to whatsnew |
|
I've rerun the failing test, it should pass in about 30 minutes. If you don't want to wait that long, you can already merge this PR (or afterwards). According to our contributing guidelines, this should be done with a merge commit. GitHub's automatic commit message for that is fine, but please change the body of the commit message to say something like "Update & fix units and size of the water module". |
| Next release | ||
| ============ | ||
|
|
||
| Water/Nexus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Wegatriespython @glatterf42 while reviewing #388 I noticed that this heading and the bullet list below have a leading indent of two spaces. In the built HTML docs, the heading does not appear and the list items don't match the rest of the list.
I'll correct the change in a commit on that branch, to align with the style used for previous releases.
In the future, please closely eyeball docs additions to make sure they render as you intend. The rest of the same file can be an example for formatting.
- Subsections for specific modules follow general list. - Remove leading indent. - Omit .model prefix from :mod:`…` links. - Use nested lists for 2+ changes to the same submodule. - Explain the acronym 'MCM', since this does not appear elsewhere in the documentation.
- Subsections for specific modules follow general list. - Remove leading indent. - Omit .model prefix from :mod:`…` links. - Use nested lists for 2+ changes to the same submodule. - Explain the acronym 'MCM', since this does not appear elsewhere in the documentation.
Update doc/whatsnew.rst Co-authored-by: Fridolin Glatter <[email protected]>
- Subsections for specific modules follow general list. - Remove leading indent. - Omit .model prefix from :mod:`…` links. - Use nested lists for 2+ changes to the same submodule. - Explain the acronym 'MCM', since this does not appear elsewhere in the documentation.
Update doc/whatsnew.rst Co-authored-by: Fridolin Glatter <[email protected]>
- Subsections for specific modules follow general list. - Remove leading indent. - Omit .model prefix from :mod:`…` links. - Use nested lists for 2+ changes to the same submodule. - Explain the acronym 'MCM', since this does not appear elsewhere in the documentation.
Update doc/whatsnew.rst Co-authored-by: Fridolin Glatter <[email protected]>
- Subsections for specific modules follow general list. - Remove leading indent. - Omit .model prefix from :mod:`…` links. - Use nested lists for 2+ changes to the same submodule. - Explain the acronym 'MCM', since this does not appear elsewhere in the documentation.
Motivation
Changes :
How to review
PR checklist