Add reduced basin functionality#432
Conversation
582f09f to
3c16023
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #432 +/- ##
=======================================
+ Coverage 69.5% 69.6% +0.1%
=======================================
Files 300 300
Lines 24598 24771 +173
=======================================
+ Hits 17104 17260 +156
- Misses 7494 7511 +17
🚀 New features to boost your workflow:
|
|
I did not have time to check all the "how to review" points yet. would it be possible to simplify a bit, given that this is secondary functionality. and at the same time make sure contradictory option are not possible.
|
|
On your two points:
Updated the PR description to clarify the option relationships and show only the new CLI options. |
There was a problem hiding this comment.
I think this is a useful feature, both for quickly testing new feature on the water module without having extremely long runs, and for application that focus on specific regions (e.g. Eurpoean projects), where we can neglect the energy-water dynamics in other continents and reduce run time.
To me the proposed changes look good. I would ask to please update the doc/water files, in particular index.rst, to update the cli command options and, if possible check and update obsolete text and reference to utility functions.
75c471e to
f437b34
Compare
|
@adrivinca @khaeru All outstanding comments are addressed, hopefully once the test suite runs it should be ready! |
adrivinca
left a comment
There was a problem hiding this comment.
To me it looks good, also thanks for the documentation udates.
Just nee to clarify why tests fail, maybe rebase needed?
@adrivinca @Wegatriespython —this failure is new and not related to this PR. See #469 (comment) and #482, just opened, which should resolve it. After I merge that one commit to |
Now merged. I'll update this branch with rebase. |
Use basin filtering everywhere More filtering More filtering Fix accidental removal Add basin reduction options Clean up filtering code Add min 1 basin region pair req Add reg_to_basin return tech consolidate return logic Fix failing tests Tests failure due to missing valid_basins list. Adding list to context to fix issue.
filter_list now augments the automatic selection (first_k or stress) rather than replacing it. Docstring updated to document the two-step design. Test parametrized over both selection modes.
- utils.py: groupby().apply() drops groupby column in pandas 3.0; replace with cumcount() boolean mask - demands.py: cast StringDtype columns to object before xr.Dataset() - water_supply.py: filter delineation and e-flow CSVs to valid_basins in add_e_flow (mirrors read_water_availability pattern)
Centralize region→type_reg mapping in water_params() factory. Replace all inline param dicts. Add reduced_basin=True variants across data tests. Refactor test_irrigation to use water_context fixture.
khaeru
left a comment
There was a problem hiding this comment.
I have not looked in detail at the code, but appreciate the expanded tests covering this new functionality and I see that comments from @adrivinca have been addressed.
Two comments that do not block merging:
- I found the text/examples (under "Design" and "Python API") in the PR description to be helpful. In the future it would be great to simply put such examples in the documentation directly. That way, people finding and reading the docs do not need to hunt back for older PRs to get a complete description of how the code is supposed to work.
- The "Python API" example seems to indicate there is a growing collection of Context settings that are recognized by
.model.water. It may make sense to add a class like.model.water.config.Config, analogous to.model.transport.config.Configor.report.config.Config, that collects all settings that are internal to this module. Then one instance of this class can be stored/expected atContext.water.
This will be good to merge once CI checks all pass.
The one job macos-latest-py3.14-upstream-main has timed out a few times. This is a 'flakiness' in the test suite (#391) not related to the changes in this PR, so I will merge despite it. |
I think I am missing some context here: this PR/branch was merged in March, and I don't see new commits on it. Does the comment from Monday refer to something else? |
|
Oops had this PR open on the side, had meant to post the reply on #479. Sorry for the confusion. |
Reduced basin filtering for local nexus development
Adds basin filtering to enable running the nexus module locally for development.
Reduced basin mode is not intended for production, basin reduction is not
physically realistic. However for development of scenarios, model capabilities or policy design, the reduced basin scenario needs to give a good proxy of the whole model, hence the granular control over its implementation.
Addresses #414. Builds on #405 (now merged).
Design
Basin selection is two-step and compositional:
first_k(head N from CSV order)or
stress(N basins sampled across the demand/supply ratio spectrum).to the automatic set (union, not replacement). E.g. automatic gives
{A1, B1, C1}, filter_list [B1, B2, B3] gives {A1, B1, B2, B3, C1}.
All regions are guaranteed coverage by the automatic step.
New CLI options (on
mix-models water-ix nexus)Python API
Files changed
utils.pyfilter_basins_by_region()(two-step selection),compute_basin_demand_ratio(),_select_by_stress(),_diversity_select()cli.pynexus_clibuild.pyfilter_basins_by_region()inmap_basin(), storescontext.valid_basinsdemands.py,infrastructure.py,irrigation.py,water_supply.pycontext.valid_basinsconftest.pysetup_valid_basins()helper for teststest_utils.pyHow to review
filter_basins_by_region()inutils.py- verify the two-stepselection and the additive union of filter_list.
compute_basin_demand_ratio()and_select_by_stress()-verify demand/supply ranking and diversity sampling.
context.valid_basinsfiltering is appliedconsistently across demands, infrastructure, irrigation, water_supply.
cli.py.PR checklist