Conversation
… strt A lake commonly intersects a grid cell in multiple polygon pieces (e.g. straight from nlmod.grid.gdf_to_grid). lake_from_gdf turned each row into its own VERTICAL connection over the full cell area with bedleak = 1/clake, multiply-counting the lake-aquifer exchange. Pieces of one lake within a cell now collapse to a single connection with clake = cell_area / sum(piece_area / clake), the same area-weighted aggregation nlmod.gwf.surface_water.aggregate applies to RIV and DRN celldata. Numeric per-lake settings are now compared with np.allclose(rtol=1e-8) so aggregated inputs carrying float-level noise (e.g. area-weighted stages) are not rejected by the single-value check. Both regression tests fail on unchanged dev: three connections with bedleak 0.1 instead of two with area-weighted bedleak, and an AssertionError on a 1e-12 strt difference.
OnnoEbbens
left a comment
OnnoEbbens
left a comment
There was a problem hiding this comment.
Thanks Bas, hadn't realised this was happening for lakes. I have just some small textual comments.
Another thing is that I had a bit of a hard time understanding the PR text. It only clicked after I looked into the code. It looks as if it is AI generated. Not necessarily something I oppose but maybe the AI can make it more concise next time :)
| the first piece per cell. | ||
| """ | ||
| if "geometry" not in lake_gdf.columns or lake_gdf.geometry.isna().any(): | ||
| raise ValueError( |
There was a problem hiding this comment.
I had a bit of trouble reading the error message. Is this a good alternative:
"found multiple elements of one lake in a single cell; provide polygon "
"geometries of the lake to aggregate these elements"
| lakeno : with the number of the lake | ||
| strt : with the starting head of the lake | ||
| clake : with the bed resistance of the lake | ||
| A lake may have multiple rows (polygon pieces) per cellid, e.g. straight |
There was a problem hiding this comment.
| A lake may have multiple rows (polygon pieces) per cellid, e.g. straight | |
| A single lake may have multiple elements (polygon pieces) in one cell, e.g. straight |
OnnoEbbens
left a comment
OnnoEbbens
left a comment
There was a problem hiding this comment.
Thanks Bas, hadn't realised this was happening for lakes. I have just some small textual comments.
Another thing is that I had a bit of a hard time understanding the PR text. It only clicked after I looked into the code. It looks as if it is AI generated. Not necessarily something I oppose but maybe the AI can make it more concise next time :)
There was a problem hiding this comment.
This is a good improvement. Existing models will change, as the bedleak (conductance) at the edge of lakes is now scaled by the part of the cell that is covered by the lake. This is a good thing though, and will maybe help with model convergence as well.
When Onno's comments are addressed, we can merge this.
|
Thanks both! I applied Onno's suggestions, merged the latest dev (CI should pass again) and shortened the description. |
3 participants
lake_from_gdfcan now take the output ofnlmod.grid.gdf_to_griddirectly, even when one lake has several pieces in the same cell.Problem
Change
surface_water.aggregatealready does the same for RIV and DRN.Input with one row per cell per lake gives the same result as before. At lake edges, where a cell is only partly covered, the exchange is now scaled to the covered area, so existing models may change there.
Tests: two new tests in
test_013_surface_water.py, both failing ondevbefore this change.