-
Notifications
You must be signed in to change notification settings - Fork 41
Updating cooling constraints for SSP runs #268
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
=======================================
- Coverage 78.3% 77.4% -0.9%
=======================================
Files 217 217
Lines 16851 16913 +62
=======================================
- Hits 13196 13106 -90
- Misses 3655 3807 +152
🚀 New features to boost your workflow:
|
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.
Looks good to me, thanks :)
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.
So this is a for
inside a for
inside a for
? I'm surprised that this is the faster solution, it doesn't seem very efficient. Without spending more time, I'm not sure how to improve this, though, and if you say it's an improvement, then it's fine with me :)
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 thing is that for any item to be removed, the message_ix_models.model.build.apply_spec()
checks if elements exist and proceeds or says that there are no elements. This is very slow.
now we just exclude in advance sets from the remove list that we know are not in the scenario, so that they do not get sent to apply_spec()
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 this is a serious problem and/or you know a solution for it, please record it in a new issue. This will raise awareness and eventually lead to a solution :)
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.
@glatterf42 @adrivinca —I see there was previously an approving review here, but:
- I don't understand what the title means.
- The tests appear to be failing.
- The PR checklist is missing items and is not complete.
- The branch needs to be rebased.
So I am just leaving this review to request/record that these things need to be addressed so we can merge this PR.
Hi, there has been more things I added after and more to come. i can update the description and notify where everything is ready for review. |
some calibration scenarios run 2020 and 2025 free, the previous code should in theory no break, but it is safer to look at fully historical years
@glatterf42 I think now the PR is ready to be reviewed. @khaeru I see there is a requested change from you, but maybe it's obsolete? I cannot see the content |
not sure why the |
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.
Looks good in general, thanks :)
A few comments in-line and I'll check the tests next :)
message_ix_models/model/water/data/pre_processing/calculate_ppl_cooling_technology_shares.R
Outdated
Show resolved
Hide resolved
@@ -271,6 +278,152 @@ def multiply_electricity_output_of_hydro( | |||
return report_iam | |||
|
|||
|
|||
def pop_water_access(sc: Scenario, reg: str, sdgs: bool = False) -> None: |
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.
def pop_water_access(sc: Scenario, reg: str, sdgs: bool = False) -> None: | |
def pop_water_access(sc: Scenario, reg: str, sdgs: bool = False) -> pd.DataFrame: |
Not sure how mypy accepted this, but the function clearly returns a pd.DataFrame
and not None
. Is mypy enabled for the water module?
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.
It is not:
message-ix-models/pyproject.toml
Lines 113 to 117 in a45fce4
[[tool.mypy.overrides]] | |
module = [ | |
# TODO Satisfy mypy in the following | |
"message_ix_models.model.material.*", | |
"message_ix_models.model.water.*", |
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 checking, that's a shame 😐
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.
With a new colleague (Vignesh) coming onboard to work on nexus/water topics, perhaps some of the added bandwidth could be reserved for improving code quality through that and other TODOs.
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, but in theory, if I have the extension on VSC should it do my typewriting itself?
I have mypy type checker (the open source mypy breaks..)
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'm also using the mypy type checker extension :)
But I don't see anywhere in their docs that they're automatically adding types to something. I do also have the Pylance extension and this infers some types even for untyped functions in the signature help it displays when hovering over a function name (for example). However, this does not mean that these functions are type checked! Pylance is just guessing the types and nothing makes sure these guesses are enforced. So if you want your functions to have type hints (for all the many good reasons), you have to add them manually, I think.
But as I mentioned before: you already spell out which types the parameters should have in the docstring, so it's not a lot of work :)
Did you check the error message produced by pytest? E.g. on ubuntu-latest-py3.13-upstream-main (which I consider the most important check), I see this: _____ ERROR collecting message_ix_models/tests/model/water/test_report.py ______
ImportError while importing test module '/home/runner/work/message-ix-models/message-ix-models/message_ix_models/tests/model/water/test_report.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../.local/share/uv/python/cpython-3.13.2-linux-x86_64-gnu/lib/python3.13/importlib/__init__.py:88: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
message_ix_models/tests/model/water/test_report.py:8: in <module>
from message_ix_models.model.water.report import report_full
message_ix_models/model/water/report.py:11: in <module>
from message_data.tools.post_processing.iamc_report_hackathon import (
E ModuleNotFoundError: No module named 'message_data' So the error happens while pytest tried to import |
Just to comment on the issue about the legacy reporting:
|
Thanks, concerning the reporting. the module does not require a specific version of the legacy. This is just based on what scenario is the nexus module attached to. to report the water part the new reporting is used. Actually I would combine 2 and 3. I think 3 alones would need the formulation of 2 for completeness |
thanks for the quick feedback and review! now test pass. apart from the codecov check. Please let me know if this can be merged today for @OFR-IIASA to be able to run simply rebasing |
Move adjustment at the end of the script, for all technologies
f97442d
to
0f1ef22
Compare
I've rebased the branch to rewrite some commit messages. Please note for the future that we want all commits to start with a capitalized verb, e.g. "Fix xyz", not "Fixing xyz". |
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.
Yes, good to go from my point of view. I will merge and then rebase the branch for #235.
Please chime in @khaeru
Re: time needed to write tests and maintain/improve coverage, per my comment above I think this is something that's a consequence of higher-level decisions, in terms of (a) having enough colleagues available to work on it and (b) having a greater (or non-zero) portion of their time reserved to work on it (as opposed to other tasks).
If that broader context doesn't make it possible, we can't change that immediately. We can only point out that the resulting decrease in coverage means the code is harder and more costly to use and adapt. The best way to reduce that high friction/inefficiency is to reserve time for work on code quality.
This PR seems to have triggered this message_ix_models/model/water/data/water_for_ppl.py:283:5: C901 `cool_tech` is too complex (12 > 11)
|
281 | # water & electricity for cooling technologies
282 | @minimum_version("message_ix 3.7")
283 | def cool_tech(context: "Context") -> dict[str, pd.DataFrame]:
| ^^^^^^^^^ C901
284 | """Process cooling technology data for a scenario instance.
285 | The input values of parent technologies are read in from a scenario instance and I am not sure why this wasn't picked up by the same code quality CI job on the branch here, which passed. So two actions, for which we can have separate issues/PRs:
|
As for investigating the workflow: The |
We identified that there's a typo in the YAML file, note
Fixing the latter to match the former should hopefully resolve the issue. |
hi, I am a bit lost. is it a fix needed in the water module or in pytest? |
@glatterf42 has kindly offered to make the fix to the workflow, and also modify the |
**Changes in cooling capacity constraints for SSP update
Fixes for the SSP update.
remove.set
as currently it takes lot of time just to check if the set is thereHow to review
check testes, suggest form edits
PR checklist