Closed measrainsey closed 1 week ago
Attention: Patch coverage is 97.67442%
with 1 line
in your changes missing coverage. Please review.
Project coverage is 75.6%. Comparing base (
a073f64
) to head (6e2ba3b
). Report is 8 commits behind head on main.
Files with missing lines | Patch % | Lines |
---|---|---|
message_ix_models/util/click.py | 94.1% | 1 Missing :warning: |
hi! some notes:
tools.costs
into water
, we want to allow for the specification of SSP scenario in building water
. for the time being, i have created a new parameter in util.click
called ssps
that is optional and called upon using --ssps
. the reason why i didn't use the existing ssp
parameter is i found that it changed the CLI command of mix-models water-ix
quite a bit, and i also wanted the parameter to be Optional
instead of an Argument
. i didn't feel comfortable directly modifying the ssp
parameter since i wasn't sure if doing so could cause some code that uses the ssp
parameter to break elsewhere.csp_sm1_ppl
, csp_sm2_ppl
, and csp_sm3_ppl
are not included in the correct tools.costs
input files for the cooling
submodule, so therefore these cooling technologies are now missing. i'll let @adrivinca decide if it's okay to not have these; otherwise we can add them into the input files in tools.costs
so they are accounted for.i'll be away for 2 weeks so won't be very responsive to any reviews/feedback during this time, but i can address any comments upon my return. thank you!
Since @measrainsey is away, I'll update this with rebase after the merge of #242.
EDIT: Seems that failed, so someone will have to do a manual rebase.
With Meas we were trying to add the SSP dimension as an input variable to our scenarios and configuration.
For the water model we usually have three functions, water_ini
where we initiate some instances of context
that could be used later on; then the functions nexus
and cooling
plus sibling '_cli' functions.
We are not sure if SSP is defined already as a common parameter like regions (as I think to remember in the past, but I don't find the source now), and it could be simply defined with @common_params("ssps")
, or if we should introduce it like we do for the RCP or SDG options.
Also, it seems now that in the current script Meas is assigning context.ssps
in water_ini()
like we do for other objects of context, but if I try to read and print it it results being empty "None".
mix-models --url=ixmp://ixmp_dev/clone_new_SSP2/test_water water-ix --regions=R12 --ssps=SSP1 cooling
...
print(
"Running code until this point: SSP: ",
context.ssps,
"regions:",
context.regions,
"time:",
context.time,
)
...
> Running code until this point: SSP: None regions: R12 time: ['year']
While, apparently to make the code work I need to write mix-models --url=ixmp://ixmp_dev/clone_new_SSP2/test_water water-ix --regions=R12 --ssps=SSP1 cooling --ssp=SSP2
so probably we are doing something wrong there with the use of water_ini
or the definition of SSP.
I was wondering if other eyes could help us spot the issue, like @glatterf42 or @khaeru if have time.
Thanks!
@adrivinca I will try to take a look tomorrow. I am not entirely clear on what your requirement is here, but my hunch is that it can be resolved by modifying the existing CLI option or how you use it, and does not require an entire duplicate.
Okay, I've added a new function/decorator @scenario_param(…)
that, according to the options given, can either create a (mandatory, positional) click.Argument or a click.Option. Here's the preview build of the documentation.
I'll next do the following:
Okay, now done. Let's see if the checks pass.
I had another thought while looking at this code. It seems like there are a number of settings that the .water
code expects to see:
https://github.com/iiasa/message-ix-models/blob/0645fd54d2a6a7346fd520d1f1d223957579f59a/message_ix_models/tests/model/water/data/test_water_for_ppl.py#L90-L98
One pattern we are trying to use more often is to have module-specific Config
classes for each module that collect settings specific to that model.
There are few reasons for doing this. One is to allow for the real or imaginable possibility that someone says "Run the .water
code with its SSP1 settings on an SSP2 base scenario." If the former setting is stored as e.g. context.water.ssp
, then this can be done without much trouble.
There are a number of examples of Config classes now in the code base, but if you like I can help set one up and show how to adapt the code.
Hi all, then, as discussed with Paul, we will address any possible changes to the config in a separate PR and we can merge this one. thank you!
we can merge this one. thank you!
I see that the PR checklist is not complete, and we haven't had anyone requested to review (thus no completed review). @adrivinca would you prefer that…
I updated the introduction, checked the work, rebased and updated the whatsnew file. Now it seems all good apart from one test of an older python version for mac...
Thanks! If you go to the details of that failed test, you will find the following lines:
___________ message_ix_models/tests/tools/costs/test_projections.py ____________
[gw2] darwin -- Python 3.11.9 /Library/Frameworks/Python.framework/Versions/3.11/bin/python
worker 'gw2' crashed while running 'message_ix_models/tests/tools/costs/test_projections.py::test_bare_res[R12]'
Which tell you that this is not your fault at all, the computer handling this test simply crashed. So I restarted the test and expect it to complete :)
So do I understand correctly that you want us to review this PR again, now?
I think it could be merged asap Meas changes were minor, but if you want to quickly review Paul changes, please go ahead
I'll merge as soon as the tests complete after the rebase.
This PR changes the code in the
water
model to use the regionally-differentiated and scenario-specific cost projections created bytools.costs
.Currently the costs for cooling technologies are being read from an input CSV and kept constant over the time horizon.
This PR's branch is derived from the
wat_SSP_upd2
branch, so this branch can be rebased once #242 is closed or merged into that branch.How to review
For @adrivinca: Check if the implementation is correct and if the outputs make sense.
For @khaeru and/or @glatterf42 : Read the diff and note that the CI checks all pass.
PR checklist