Skip to content

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Feb 26, 2025

Issue addressed

No issue created.

Adds two new methods for water demands:

  • setup_domestic_demand_from_population
  • setup_irrigation_from_vector

It also adds an extra argument to setup_allocation_areas to filter subbasins that would be too small (eg if a river meanders very closely to another admin area then very small allocation areas can be created).

Explanation

I already encountered twice that local irrigated areas are in vector format rather than raster and we have a good method in core to treat vector data (grid_from_geodataframe with method fraction) so that avoids to loose too much data for a user that would first need to rasterize the map themselves and risk loosing info.

Secondly I think getting demand based on consumption per capita was already on the wishlist and it already poped up for the same study so I went ahead and added. In another version, I guess we may want to have these statistics stored in a admin area shapefile rather than a constant value but this will be a new feature for later.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed

Additional Notes (optional)

I should have done it in another PR but while working on the same model, I realised that computing river depths in setup_rivers using manning or gvf methods was apparently broken for two years....
And while this PR is still open for review, I also added that parameters can be derived based on soil texture mapping with default values for InfiltCapSoil. I'll put the references (USDA, FAO) when we update the v1 docs together with the review of parameters for landuse.

Copy link
Collaborator

@JoostBuitink JoostBuitink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work and great additions! Looks good to me, just a couple of small typo's and questions, after that we are good to merge I think!

elevtn_map="dem_subgrid",
)

assert "RiverDepth" in mod.grid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets say the manning method did produce a RiverDepth map, but the gvf method did not. Will this test capture that? I think it will not flag it right, since the RiverDepth is already part of the grid due to the previous method.

In addition, should we also do a similar test for the powlaw method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the best test but if there is any error (eg missing dem_subgrid map which was the curretn issue) then the method itself will error. So the assert is not really the goal but justchecking that the method runs without errors. For powlaw we are testing when we build models as this is the default method. Is this test okay then? Or do you want me to test further with some values check or other?

@hboisgon hboisgon requested a review from JoostBuitink March 12, 2025 01:50
@savente93
Copy link
Contributor

Discussed this with Hélène, the one remaining comment is not to bad, I just added an explanation as a comment for future reference. Since Joost seems to be out of office, so I'll be merging this.

@savente93 savente93 merged commit df8ba84 into main Mar 12, 2025
3 of 4 checks passed
@savente93 savente93 deleted the improve_demand branch March 12, 2025 13:47
savente93 added a commit that referenced this pull request Mar 17, 2025
* add domestic from population

* add irrigation from vector

* small bugfixes

* update changelog

* small adjustment

* improve docs of setup_rivers

* bugfix river depth with manning or gvf methods

* add infiltcapsoil mapping from texture

* update after review

* proper pin of hydromt to avoid getting v1 alpha installed

* Update test_model_methods.py

* fix: linting

---------

Co-authored-by: Sam Vente <[email protected]>
@deltamarnix deltamarnix added this to the Release 0.8 milestone Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants