Skip to content

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

Merged
merged 27 commits into from
Mar 21, 2025
Merged

Updating cooling constraints for SSP runs #268

merged 27 commits into from
Mar 21, 2025

Conversation

adrivinca
Copy link
Contributor

@adrivinca adrivinca commented Dec 10, 2024

**Changes in cooling capacity constraints for SSP update

Fixes for the SSP update.

  • we noticed nuc_lc cooling technology was not correctly parametrized. I did not notice it because in all the scenarios I tested the technology was not active, but in some scenario it is, before 2020
  • historical csp technologies added in data files
  • Oliver suggested to pre-filter the remove.set as currently it takes lot of time just to check if the set is there
  • changes in constraints to the parent technologies, several were added, including initial_activity and capacity
  • added shares constraints to calibrate 2020 and 2025

How to review

check testes, suggest form edits

PR checklist

  • Continuous integration checks all ✅
  • Update docs/whatsnew.

@adrivinca adrivinca added the water MESSAGEix-Nexus (water) variant label Dec 10, 2024
@adrivinca adrivinca self-assigned this Dec 10, 2024
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 67.91667% with 77 lines in your changes missing coverage. Please review.

Project coverage is 77.4%. Comparing base (a45fce4) to head (0f1ef22).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
message_ix_models/model/water/report.py 9.0% 70 Missing ⚠️
...essage_ix_models/model/water/data/water_for_ppl.py 95.5% 4 Missing ⚠️
message_ix_models/model/water/data/water_supply.py 50.0% 2 Missing ⚠️
message_ix_models/model/water/cli.py 0.0% 1 Missing ⚠️
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     
Files with missing lines Coverage Δ
message_ix_models/model/water/build.py 94.9% <100.0%> (+1.5%) ⬆️
message_ix_models/model/water/data/demands.py 78.8% <100.0%> (+0.2%) ⬆️
...odels/tests/model/water/data/test_water_for_ppl.py 100.0% <100.0%> (ø)
message_ix_models/model/water/cli.py 34.5% <0.0%> (ø)
message_ix_models/model/water/data/water_supply.py 76.3% <50.0%> (-0.3%) ⬇️
...essage_ix_models/model/water/data/water_for_ppl.py 94.7% <95.5%> (-0.6%) ⬇️
message_ix_models/model/water/report.py 9.1% <9.0%> (+1.2%) ⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@adrivinca adrivinca requested a review from glatterf42 December 10, 2024 13:28
Copy link
Member

@glatterf42 glatterf42 left a 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 :)

Copy link
Member

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 :)

Copy link
Contributor Author

@adrivinca adrivinca Dec 10, 2024

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()

Copy link
Member

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 :)

Copy link
Member

@khaeru khaeru left a 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.

@adrivinca
Copy link
Contributor Author

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.
thanks

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
@adrivinca adrivinca changed the title Wat ssp upd4 Updating cooling constraints for SSP runs Mar 19, 2025
@adrivinca
Copy link
Contributor Author

@glatterf42 I think now the PR is ready to be reviewed.
let's see if texts work, I removed one because I got rid of the tested function, but added a new one.

@khaeru I see there is a requested change from you, but maybe it's obsolete? I cannot see the content

@adrivinca
Copy link
Contributor Author

adrivinca commented Mar 19, 2025

not sure why the message_ix_models/tests/model/water/test_report.py test gives an error, in theory it is marked to be skipped with @pytest.mark.xfail(reason="Temporary, for #106")

Copy link
Member

@glatterf42 glatterf42 left a 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 :)

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not:

[[tool.mypy.overrides]]
module = [
# TODO Satisfy mypy in the following
"message_ix_models.model.material.*",
"message_ix_models.model.water.*",

Copy link
Member

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 😐

Copy link
Member

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.

Copy link
Contributor Author

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..)

Copy link
Member

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 :)

@glatterf42
Copy link
Member

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 model/water/report.py, which it does before trying to run the actual tests. And the error message seems pretty clear: message_data is not installed and it shouldn't be: message-ix-models is open source, while message_data is closed source, so message-ix-models should not depend on message_data if we want to keep it that way. That is why we moved one version of the reporting in message_data to message-ix-models. If this version of the function doesn't suit you, feel free to adapt it or rewrite it's behaviour using the new and improved genno-based system, but don't import it from message_data.

@khaeru
Copy link
Member

khaeru commented Mar 20, 2025

Just to comment on the issue about the legacy reporting:

  • There is probably a big delta (no one knows) between the version of .iamc_report_hackathon on the message_data SSP_dev branch and the one on message_ix_models main.
  • As @glatterf42 says, it is best to update the latter to have the desired behaviour.
  • However, if that would be a huge and unworkable expansion of scope for this PR (probably the case), and if message_ix_models.model.water.report strictly requires the version from a message_data branch, there are some things that can be done:
    1. Say clearly in the documentation that “This code only works with the iamc_report_hackathon version on [BRANCH] in [REPO], which does [X, Y, Z]; it does not work with the version in message-ix-models, which produces [SPECIFIC UNWANTED OUTCOME].”
    2. Use a try/except block or if statement around the import, such as:
      from message_ix_models.util import HAS_MESSAGE_DATA
      
      if HAS_MESSAGE_DATA:
          from message_data.tools.post_processing.iamc_report_hackathon import (
              report as legacy_report,
          )
      else:
          log.warning(f"Missing message_data; code in {__name__} will not work")
      or:
      try:
          from message_data.tools.post_processing.iamc_report_hackathon import (
              report as legacy_report,
          )
      except ImportError:
          log.warning(
              "Missing message_data.tools.post_processing; fall back to "
              "message_ix_models.report.legacy. This may not work."
          )
          from message_ix_models.report.legacy.iamc_report_hackathon import (
              report as legacy_report,
          )
    3. Instead of (ii) move the import from the module top-level to inside a particular function (seems like run_old_reporting()) where it is used. This way the ImportError does not happen unless and until someone calls that particular function.

@adrivinca
Copy link
Contributor Author

adrivinca commented Mar 20, 2025

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.
but I am not aware of the compatibility of the message_ix_models legacy_report with the most recent MESSAGE scenarios. Therefore I would keep the connection to message_data, using one of the suggestions from Paul.

Actually I would combine 2 and 3. I think 3 alones would need the formulation of 2 for completeness

@adrivinca
Copy link
Contributor Author

thanks for the quick feedback and review! now test pass. apart from the codecov check.
and yes, we will work with Vignesh to improve the code in several parts, hopefully in the reporting too. But probably we will start from other sections (not cooling technologies) that are more urgent for other projects.

Please let me know if this can be merged today for @OFR-IIASA to be able to run simply rebasing ssp-dev.
Thank you!!

@glatterf42
Copy link
Member

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".
Next, I looked at the missing lines to see if we could easily cover them with tests. Unfortunately, that does not seem to be the case since we still don't have what you promised in #106: a test scenario set up in such a way to run the water code on it. So from my view, we could merge the PR without tests again, but I don't want this to become a theme: opening a PR, working on it for some time, and then postponing the tests because the PR should ideally be merged ASAP. So I'm inclined to say: for this one, it's okay one last time, but the next water PR should add tests as promised about two years ago (#88 and #93). Please chime in @khaeru (and adjust your requested changes if you think we can now merge this PR).

Copy link
Member

@khaeru khaeru left a 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.

@khaeru khaeru merged commit a0d5420 into main Mar 21, 2025
24 of 25 checks passed
@khaeru khaeru deleted the wat_SSP_upd4 branch March 21, 2025 09:16
@khaeru
Copy link
Member

khaeru commented Mar 25, 2025

This PR seems to have triggered this ruff error:

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:

  • Reduce complexity of cool_tech() (preferred) or add an exemption inline or in pyproject.toml.
  • Investigate the way the workflow functions and see if there is anything to be fixed.

@glatterf42
Copy link
Member

As for investigating the workflow: The action/checkout step shows that a45fce49fe3aa17ddb9d83f8e5b550accc0090b4 was checked out, which is a commit on main. So maybe we just didn't adjust the code quality check and this doesn't use a branch's version of the code, but the one on main?

@khaeru
Copy link
Member

khaeru commented Mar 26, 2025

We identified that there's a typo in the YAML file, note outputs vs output:

ref: ${{ needs.check.outputs.ref }}

ref: ${{ needs.check.output.ref }}

Fixing the latter to match the former should hopefully resolve the issue.

@adrivinca
Copy link
Contributor Author

hi, I am a bit lost. is it a fix needed in the water module or in pytest?
or will it imply fixes in both?
thanks

@khaeru
Copy link
Member

khaeru commented Mar 26, 2025

hi, I am a bit lost. is it a fix needed in the water module or in pytest?
or will it imply fixes in both?
thanks

@glatterf42 has kindly offered to make the fix to the workflow, and also modify the cool_tech() function so it is not too complex (the function isn't wrong/broken, it's just more complicated and would be hard to maintain). I guess he will ask for your review once those changes are in a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
water MESSAGEix-Nexus (water) variant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants