Water demand #226
Water demand #226
Conversation
code also useful for rootingdepth, but should require additional information before being useful for wflow
|
Thanks for adding irrigation and crop factor maps @JoostBuitink ! For a test version I think it would be good to also add a soil profile for irrigated paddy areas: |
There was a problem hiding this comment.
Nice developments @dalmijn and @JoostBuitink ! And a good start to get demand data from pcr_globwb or irrigation/paddy areas!
I reviewed mainly the functions from @dalmijn, as I don't think setup_irrigation is fully done yet.
There's a few comments and suggestions here and there. Maybe one function I did not fully understand is how the allocation areas are derived so would be happy to discuss further!
hydromt_wflow/wflow.py
Outdated
| def setup_allocation( | ||
| self, | ||
| min_area: float | int = 0, | ||
| admin_bounds_fn: str = "gadm", |
There was a problem hiding this comment.
We are cleaning this up still for wflow but we do not want to assume a default dataset value anymore, they are just mandatory arguments :) the example from artifact_data can be mentioned in the examples. Which might be good to add here! (update water demands example to showcase the new methods)
hydromt_wflow/wflow.py
Outdated
| self, | ||
| min_area: float | int = 0, | ||
| admin_bounds_fn: str = "gadm", | ||
| admin_level: int = None, |
There was a problem hiding this comment.
This is too specific to gadm and people should be able to use their own geometry. I think in the data catalog, it's actually gadm_level1 that you can call so just use this in admin_bounds_fn (in testing / examples).
Possibly you can add **kwargs here to the get_geodataframe method so that a user is able to select a specific layer in their data via the driver_kwargs (if they did not do that in their own data catalog)
| self.logger.info("Preparing water demand allocation map.") | ||
| # Will be fixes but for know this is done like this | ||
| # TODO fix in the future | ||
| admin_bounds = None |
There was a problem hiding this comment.
So you accept that admin_bounds can actually be None? Then this is your default value in the function parameters admin_bounds_fn.
I will read the method further but if None do you use the whole basin as one?
You could also add a new method to derive subbasins based on maximum area or pfafstetter or streamorder? See this issue in core Deltares/hydromt#683
You could then have:
- setup_allocation_areas _from_geodataframe
- setup_allocation_areas_from_subbasins
There was a problem hiding this comment.
A good idea, but maybe for a future improvement.
hydromt_wflow/wflow.py
Outdated
| @@ -2494,6 +2498,292 @@ def setup_rootzoneclim( | |||
| # update config | |||
| self.set_config("input.vertical.rootingdepth", update_toml_rootingdepth) | |||
|
|
|||
| def setup_allocation( | |||
There was a problem hiding this comment.
| def setup_allocation( | |
| def setup_allocation_areas( |
To match better wflow names. Makes it also clearer for me that you are only preparing areas here not demand or actual allocation data.
hydromt_wflow/wflow.py
Outdated
|
|
||
| def setup_non_irigation( | ||
| self, | ||
| non_irigation_fn: str = "pcr_globwb", |
There was a problem hiding this comment.
Either None if optional or required argument
| non_irigation_fn: str = "pcr_globwb", | |
| non_irigation_fn: Union[str, Path, xr.Dataset], |
hydromt_wflow/workflows/demand.py
Outdated
| sub_basins = sub_basins.dissolve("uid", sort=False, as_index=False) | ||
| _count += 1 | ||
|
|
||
| alloc = full_like(ds_like["dem_subgrid"], nodata=-9999, lazy=True).astype(int) |
There was a problem hiding this comment.
Why dem_subgrid? If kinematic wave models, I'm not event sure the map is available... "wflow_dem" or "wflow_ldd" or "wflow_subcatch" are safer as they should always be there (especially wflow_subcatch that you should actually use rather than self.basins actually as self.basins is unfortunately basins at merit_hydro resolution rather than wflow model resolution...)
There was a problem hiding this comment.
Updated the code to no longer rely on dem_subgrid. Other remark about self.basins vs. wflow_subcatch is still open I guess
| ds_like=self.grid, | ||
| min_area=min_area, | ||
| admin_bounds=admin_bounds, | ||
| basins=self.geoms["basins"], |
There was a problem hiding this comment.
See comment in workflow safer to rasterize wflow_subcatch in in ds_like
There was a problem hiding this comment.
I do not follow you here.
There was a problem hiding this comment.
I guess this is related to the high resolution geometry, which can cause issues (like I showed). So we can either use the geometry of the basin at wflow resolution (once that PR is merged), or do something like this for now self.grid["wflow_subcatch"].raster.vectorize()
There was a problem hiding this comment.
Right. Yes, that indeed will be solved in the other PR.
| ds : xr.Dataset | ||
| _description_ | ||
| """ | ||
| # Reproject to up or downscale |
There was a problem hiding this comment.
Can you else reproject densities? eg m3/m2 and then re multiply by the smaller or bigger cell area size from ds_like?
Useful function: hydromt.raster.area_grid or hydromt.raster.density_grid
Then you are also scaling correctly the other demands than just dom and you do not need a population map anymore?
| ds_like.raster.bounds, | ||
| ) | ||
|
|
||
| # Create dummy dataset from this info |
There was a problem hiding this comment.
I guess this is to ensure better mass conservation? Ie you are not creating or removing m3 just because or up or downscaling? Then I think it's nice to apply to not just domestic demand.
If you make a nice enough I would also move this to hydromt core. It's useful to have this kind of mass conservation reprojection for water demands or population or livestock or any count data :)
| f"input.vertical.{lname}.demand_{suffix}", | ||
| ) | ||
|
|
||
| def setup_irrigation( |
There was a problem hiding this comment.
I did not review this function and workflows yet, not sure if it is finished.
One thing while having a quick look, is about the consistency of all the datasets irrigated_area, landuse and mirca together with the already existing landuse map of wflow and other landuse paramters (setup_lulcmaps)?
I didn't check anything with mirca. but for irrigated and paddy areas, I guess good to burn in the wflow_landuse map and re-derive parameters accordingly?
There was a problem hiding this comment.
This is a good point, but it was fairly tricky to find a dataset that had both good global coverage of (high resolution) irrigated area's and a mask for paddy fields. Hence why we decided to combine a couple of different datasources. I guess we can add support for the glcnmo landcover dataset, as that contains quite high resolution data and a specific classification for rice fields.
some todo's still open, but contains all required functionality
They both point to the same map currently
|
ToDo's still open:
|
artifact data needs to be updated to include the required water demand datafiles, such that these can be used for the examples (and testing)
|
Nice work! A few comments/suggestions:
|
Issue addressed
Fixes #87
Explanation
Explain how you addressed the bug/feature request, what choices you made and why.
Checklist
mainAdditional Notes (optional)
Add any additional notes or information that may be helpful.