Updating cooling constraints for SSP runs

[adrivinca]

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

[codecov]

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.

[glatterf42]

Looks good to me, thanks :)

[glatterf42]

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

[adrivinca]

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

[glatterf42]

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

[khaeru]

@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]

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

[adrivinca]

@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]

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

[glatterf42]

Looks good in general, thanks :)
A few comments in-line and I'll check the tests next :)

[glatterf42]

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?

[khaeru]

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.*",

[glatterf42]

Thanks for checking, that's a shame 😐

[khaeru]

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.

[adrivinca]

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

[glatterf42]

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]

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]

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]

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]

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]

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

[khaeru]

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]

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]

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]

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

message-ix-models/.github/workflows/pytest.yaml

Line 111 in a0d5420

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

message-ix-models/.github/workflows/pytest.yaml

Line 192 in a0d5420

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

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

[adrivinca]

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]

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.