Updates to the water module, water avaialbility and country sub-annual features

[adrivinca]

This PR only affects the water module with some updates:

• Global and Zambia model water availability updated to ISIMIP 3a scenarios (CMIP6)
• redefining all the paths from private_data_path package_data_path
• updates in the reporting script for country models to include the reporting of variables at the sub-annual time-steps
• changing the water cli reporting function to choose between full reporting (legacy + water) or just water reporting. in the case of country models we do not run the legacy reporting because it does not work.

How to review

Someone can try run the reporting ona test scenario made for Zambia with sub-annual timesteps
mix-models --url=ixmp://ixmp_dev/MESSAGEix_ZM/test_report water-ix --regions=ZMB report --water

PR checklist

• Continuous integration checks all ✅

[codecov]

Codecov Report

Merging #115 (f143c9a) into main (e48cc43) will decrease coverage by 1.3%.
The diff coverage is 6.5%.

@@           Coverage Diff           @@
##            main    #115     +/-   ##
=======================================
- Coverage   69.3%   68.1%   -1.3%     
=======================================
  Files         75      75             
  Lines       4975    5066     +91     
=======================================
+ Hits        3448    3450      +2     
- Misses      1527    1616     +89     

Files	Coverage Δ
message_ix_models/model/water/build.py	14.9% <50.0%> (ø)
message_ix_models/model/water/data/irrigation.py	14.8% <50.0%> (ø)
message_ix_models/model/water/utils.py	30.6% <50.0%> (ø)
...essage_ix_models/model/water/data/water_for_ppl.py	3.5% <0.0%> (ø)
message_ix_models/model/water/cli.py	32.7% <20.0%> (-0.7%)	⬇️
...ssage_ix_models/model/water/data/infrastructure.py	4.0% <0.0%> (ø)
message_ix_models/model/water/data/water_supply.py	4.8% <8.3%> (ø)
message_ix_models/model/water/data/demands.py	5.7% <2.8%> (-0.4%)	⬇️
message_ix_models/model/water/reporting.py	4.8% <4.9%> (-0.5%)	⬇️

... and 1 file with indirect coverage changes

[glatterf42]

Hey @adrivinca, thanks for the PR :)
Please make sure your code passes the linting tests before merging it. The first of those is to apply black to your code files, but isort, flake8 and mypy will need to be satisfied, too. If you are using VSCode, you can set these tools up to run automatically whenever you hit 'save' using these instructions, which makes complying with them a lot easier. Other code editors should support this, too, but I can't quickly recommend a guide for them.
Please also think about writing tests for the code you are adding since we want our test coverage to be as high as possible, ensuring that our code is reliable.

[adrivinca]

Thanks, I have some issues in setting it up in my VSC, it is not actually like in those instructions.

But I generally run isort -rc . && black . && mypy . && flake8 and still some things are missing. I understand that flake only tells me what I should do and does not do
Now for instance I have this error that I don't know how to solve

.\reporting.py:70:1: C901 'report' is too complex (25)

It complaints that there are too many if conditions, but they are somehow necessary

Concerning test, I did not add new functions and the tests of the existing functions will come with the PR #106

[glatterf42]

Yes, thanks for letting me know. Maybe I should write a small guide of my own to enable colleagues to set the tools up for automatic runs, but I will have to test my settings first.

As for the complexity, the issue is that if you include too many (nested) if conditions, the code will be very complex to understand -- for people as well as for the computer. If you were to write tests, your function would now probably require about 25 different sets of parameters to be fully tested because you need to make sure to test each if-branch. One common solution is to move if-checks to new functions, which moves complexity as well. See e.g. your lines 484-502:

    for var in elec_hydro_var:
        if "hydro_1" in var or "hydro_hc" in var:
            report_iam = report_iam.append(
                # Multiply electricity output of hydro to get withdrawals
                # this is an ex-post model calculation and the values are taken from
                # data/water/ppl_cooling_tech/tech_water_performance_ssp_msg.csv
                # for hydr_n water_withdrawal_mid_m3_per output is converted by
                # multiplying with   60 * 60* 24 * 365 * 1e-9 to convert it
                # into km3/output
                report_iam.multiply(
                    f"{var}", 0.161, f"Water Withdrawal|Electricity|Hydro|{var[21:28]}"
                )
            )
        else:
            report_iam = report_iam.append(
                report_iam.multiply(
                    f"{var}", 0.323, f"Water Withdrawal|Electricity|Hydro|{var[21:28]}"
                )
            )

Instead, you could say something like:

    report_iam = multiply_electricity_output_of_hydro(elec_hydro_var, report_iam)

    ...

def multiply_electricity_output_of_hydro(elec_hydro_var, report_iam):
    for var in elec_hydro_var:
        if "hydro_1" in var or "hydro_hc" in var:
            report_iam = report_iam.append(
                # Multiply electricity output of hydro to get withdrawals
                # this is an ex-post model calculation and the values are taken from
                # data/water/ppl_cooling_tech/tech_water_performance_ssp_msg.csv
                # for hydr_n water_withdrawal_mid_m3_per output is converted by
                # multiplying with   60 * 60* 24 * 365 * 1e-9 to convert it
                # into km3/output
                report_iam.multiply(
                    f"{var}", 0.161, f"Water Withdrawal|Electricity|Hydro|{var[21:28]}"
                )
            )
        else:
            report_iam = report_iam.append(
                report_iam.multiply(
                    f"{var}", 0.323, f"Water Withdrawal|Electricity|Hydro|{var[21:28]}"
                )
            )
    return report_iam

Of course, you could also rename the parameters to differentiate e.g. report_iam_base and report_iam only after the function call. This would move a complexity of two out of your function by always calling this auxiliary new function. Because yes, complexity is actually measured as a linear sum (which I might explain in more detail at some point in the future soon) :)

[adrivinca]

thanks @glatterf42 ! I tried to set up all the automatism I could on VSC and reduced the complexity of the report function to of 6 levels. Still it is too complex for flake, so I bypass the check there, since it is not a priority to make it the perfect script.

now I get a Lint error related to a config file not touched by me... I wonder if it is a problem of main
If there is nothing else, I wonder if we could merge with the note that tests are coming in PR #106

[glatterf42]

Good catch about mypy! They released version 1.5.0 on August 10, which changed the signature of the replace() function which we use in the line that's causing trouble here. However, there already are a number of issues/PRs related to it aiming to restore replace()'s ability to handle DataClassInstances again:

• This PR introduced the changes
• This issue should be enough to keep track of to learn about the final solution
• This comment explains technical background and was picked up in
• This PR
• This discussion keeps track of the thoughts/ideas on the topic
• This PR seems to be related, but maybe not as important for us

So my guess is we can ignore the error for the time being as it will probably be fixed by a new release of mypy soon. We could also pin version 1.4.1 until then.

[glatterf42]

The PR you mention for adding tests, #106, had it's last push about a month ago, so I don't know how soon this will actually add tests. I understand that it seems tedious to write tests for code that you will use correctly, but if I understand correctly, our aim with this repository is to make our tools public. This has multiple benefits, but one notable drawback is that people could/will start using our code however they see fit, which does not necessarily comply to the way you intended the code to be used. If and when people run into problems on that route, they will turn to us to help them. And if Paul and I don't know what they are referring to, we will ask you. For just one or two persons, this might be fine, but there will come a point where it is more time efficient to write tests that ensure behaviour now rather helping individual cases as they come up.

A very similar point can be made about code complexity; so I would ask you to please don't block the test. If you already started simplifying the code, you have a good idea how to do that now.

I realise that this time requirement is often not accounted for by projects, but if at all possible, I would prefer to get the tests and linting checks to pass before merging.

[glatterf42]

After talking to Paul, we agreed that it is fine to exclude single functions from flake8 cover for now. But please mark them with a #TODO or #FIXME so that they are easily identifiable later on.
We also agreed that there is no need to test everything yet, but wanted to ask if you could please add a test for one of your additions to the code? Maybe you could expand one of the existing tests that Muhammad is working on or maybe you could add a new one, but it would be nice to have a single test coming in with this PR.

[adrivinca]

Thanks for replying and checking with Paul.
I agree that we need to add tests and we have agreed in the team that this is @awais307 task in PR 106 before he leaves the team.
I have other tasks in the project, like making the code ready for stakeholders, either on message_ix_models, or just copying this in the LEAP-RE repository. That's why I am not setting up the test suit here and I will rather help Muhammad on the other PR.
Sorry for that and hope Muhammad jumps in soon, according to his other duties of course.

I'll still add the #TODO mark

[adrivinca]

sorry for the confusion, I reverted the last commit and I have no more to add here

[glatterf42]

No worries. Just for your information: with this PR as it is, people are able to check out the specific commit with which you added the leap_re project and thus gain access to all files you committed there even though you reverted it. If you want, we can instead remove this commit (hopefully) entirely:

git reset --hard dbe1019
git push --force

This should remove the last two commits and I'm hoping mentions of them as well.

[glatterf42]

Rebased onto current main, now all tests are passing except for the test coverage, which #106 will hopefully add soon. Please really try to increase the coverage there as we are aiming for 100% coverage with our public tools. If you agree to do that and don't have any further additions here, we can merge this PR.

[adrivinca]

yes please let's merge this. thanks a lot

[khaeru]

Hi all—please in the future try to follow the code style per commit message formatting. Please ask if you need any clarification or assistance on that.