Wat ssp upd3: third update to cooling technology, with SSP differentiation

[adrivinca]

This PR connects introduces a SSP configuration for cooling technologies, introduce share constraints solves some bugs like the inclusion of the correct cooling technologies for csp and geothermal plants.

How to review

Check that code and test are in line with the standards.
Eventually run the code on a scenario at choice and see it the model solves mix-models --url=ixmp://ixmp_dev/%model%/%scenario% water-ix --regions=R12 cooling --ssp=%SSP%, maybe @measrainsey

if we can merge by Dec 2, it can be used for the ssp updates, otherwise we'll merge directly to the ssp-dev branch.

PR checklist

• Continuous integration checks all ✅
• Add or expand tests; coverage checks both ✅
  I added these two tests:
  test_cat_tec_cooling in water/test_build.py
  and
  test_cooling_shares_SSP_from_yaml in water/data/test_water_for_ppl.py
• Update doc/whatsnew.

[please let's not delete the branch right after merging]

[glatterf42]

Already one FYI item before starting a review: the branch will automatically be deleted by GitHub after merging the PR. It will still exist on your local system, though, which is likely what you need if you want to start new work off of this branch (but keep in mind: once this branch is merged to main, you might as well start new work from there, which is even better than branching out too much). Should you really need the branch on GitHub, please clarify why and note that the automatic message that deletes branches also offers to restore them.

[glatterf42]

Thanks for this PR :)

I've left several notes and comments in-line. I know this is not our current practice, so I don't mean to blame you, but please be aware that the ideal workflow would be slightly different: you open a PR and work on it until all tests pass and only request a review once that's the case. In between those, you could even think of reviewing your code yourself to e.g. catch typos or add explanatory comments to changes that you think might warrant them.

I haven't looked at why the tests are not passing as I want to continue work on ixmp4, but please let me know if there is any particular issue you would like me to look at :)

[adrivinca]

yes sorry for the miscommunication. The PR is not ready for review yet, I should have written it

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.7%. Comparing base (464e579) to head (121885b).
Report is 126 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #256     +/-   ##
=======================================
- Coverage   76.6%   75.7%   -0.9%     
=======================================
  Files        203     203             
  Lines      15588   15704    +116     
=======================================
- Hits       11942   11902     -40     
- Misses      3646    3802    +156     

Files with missing lines	Coverage Δ
message_ix_models/model/water/build.py	93.3% <ø> (+1.9%)	⬆️
message_ix_models/model/water/cli.py	34.5% <ø> (+0.2%)	⬆️
...essage_ix_models/model/water/data/water_for_ppl.py	95.2% <ø> (+1.1%)	⬆️
...odels/tests/model/water/data/test_water_for_ppl.py	100.0% <ø> (ø)
message_ix_models/tests/model/water/test_build.py	98.1% <ø> (+1.0%)	⬆️

... and 9 files with indirect coverage changes

[adrivinca]

thanks a lot for the inputs, now I think I brought the PR in a good state, adding tests and test should pass.

[glatterf42]

Thanks for the updates and new tests, this is beginning to look good :)
Unfortunately, the tests are only passing with version 3.9.0 of message_ix or newer, the reason being that make_df() could only be used for parameters before that, not sets. This leaves you two options on how to resolve this: you could either build your own functionality for this or you can mark your function(s) (that rely on make_df() as requiring a minimum version of 3.9.0 for message_ix.

For rebuilding functionality, you could with an if check so that code that is new enough uses make_df, but older code doesn't. For this older code, you could try to manually duplicate the steps performed by make_df, but you could also just construct your own pd.DataFrame in another way.
For marking your code as requiring a minimum version, we have this decorator function already and this is how you would use it. Please keep in mind to apply this to every function calling the make_df function directly or another function that calls it indirectly, including tests.

Of course, the first approach has the advantage that your code is more compatible with other code, while the second approach may be faster to implement right now.

[glatterf42]

For this and some other .csv files, I think when I first viewed this PR, they were uploaded normally, but they now seem to be git lfs files. This is not how we want to treat csv files, see this recent comment by @khaeru. Could you please revert that?

[khaeru]

For marking your code as requiring a minimum version, we have this decorator function…

Just for reference, this info is also in the documentation, here.

The documentation points to two other functions, and then one can click "[source]" links on each of those to see how the code is used:

1. prepare_reporter(): docs -> source
2. test_prepare_reporter(): docs → source

[khaeru]

…and IMO this is the easiest and fastest thing to do, and probably not a problem unless you consciously want to support certain users who will have message_ix ≤ 3.8 and not be able to upgrade.

[adrivinca]

Thanks a lot, I did update for make_df and wanted to clean up the data files.
I noticed that a file in particular might be large (4mp) and it is not used anymore
\message_ix_models\data\water\ppl_cooling_tech/POWER_PLANTS_2016_Raptis.csv
maybe I remove this completely and just leave the others as normal csv?

[khaeru]

Yes, please go ahead to remove any data that's no longer in use. As long as it showed up on main at any point, it is never lost, and we can always go back and 'resurrect' it from the Git history as needed. In the meanwhile keeping the repo and package smaller helps things be lightweight and performant for everybody.

And non-LFS for the other CSV files: yes please.

[glatterf42]

Thanks for these fixes! As far as I can tell, the tests are now failing because the csv files stored under https://github.com/iiasa/message-ix-models/tree/wat_SSP_upd3/message_ix_models/data/water/delineation are also stored as Git LFS files. They are read in as usual csv files, but pandas just reads in the string specifying how Git LFS would load the contents of the file, not the actual contents. In light of the other changes made here, I think the best solution would be making sure that all /data/water/*.csv files are stored as plain csv files.

[adrivinca]

Hi, I noticed that all test solved after I replaced mafe_df() but then fail again in mac and Ubuntu OS after converting the LFS to normal csv files.
Thaknks Frido, I'll try replace all csv files now

Other than that I rebased, updated doc and Whatsnew, So I would be done

[glatterf42]

Thanks, this looks good to me now, provided tests are passing :)

[adrivinca]

Yes, thanks for the support @glatterf42 and @khaeru !
next steps (probably next year) will be re-organizing the config options for the function, with Paul's help and then moving to the nexus function.

[khaeru]

Please go ahead and merge if you're satisfied @glatterf42 —I don't have bandwidth at the moment to review. If #235 needs manual intervention to rebase on main after this goes in, I will help there.

[glatterf42]

Well, no objections from my side, but what about this part of the review:

Eventually run the code on a scenario at choice and see it the model solves mix-models --url=ixmp://ixmp_dev/%model%/%scenario% water-ix --regions=R12 cooling --ssp=%SSP%, maybe @measrainsey

Is this step required to be performed by someone else? If we're just verifying it solves without errors, I expect you have confirmed this already, @adrivinca? If so, we can merge, otherwise, who should confirm it?

[adrivinca]

yes, let's not to use too many people time, I have checked it myself. All good.
thanks