Real wflow basins #266
Real wflow basins #266
Conversation
There was a problem hiding this comment.
Hi Brendan, nice! And nice find for the set_precision method, rounding for geodataframe was actually not so easy not too long ago I did not realize...
Just a couple of comments but I dont think we need a really keep track of the basins_highres, it's just nice if it's in your geoms but that's it.
Could you maybe update the examples with the new basins and basins_highres geojson ? I think we are really relaxed in the tests so maybe that's why they did not complain but I think good to keep the examples up to date with this.
There was a problem hiding this comment.
Thanks for updating the examples model @dalmijn ! Not sure why the inmaps for some models got updated as well?
Also why did you add two new changes to the code? In read_tables and the index column name in basins?
You are going to hate me but I had more time to think and I actually think you got it right the first time with using basins_highres when available to filter hydrological objects such as lakes/reservoirs/glaciers. So I wonder if we should not put it back in the end for these three? In a way, it's a temporary fix for the Piave case before we address issue #262 properly but as we are talking of releasing, I'm a little uncomfortable to release without the basins high res... What do you think?
For gauges I wonder. For discharge gauges, it may help in very few cases but for precipitation it may make it worse. So maybe here keep low res basins.
| @@ -3684,7 +3700,6 @@ def basins(self): | |||
| .set_index("value") | |||
| .sort_index() | |||
| ) | |||
| gdf.index.name = self._MAPS["basins"] | |||
There was a problem hiding this comment.
Why remove this line? This would change the name of the index column from wflow_subcatch to value. Apparently we don't use this in other places in the code but why change?
There was a problem hiding this comment.
It broke the tests. It's looking specifically for the value column...
The old basin (most of the time) did not pass this and would not have it as the name anyway.
There was a problem hiding this comment.
Which test did it break? Maybe actually using wflow_subcatch rather than value as name is better no? While we are breaking behavior of self.basins anyhow :)
hydromt_wflow/wflow.py
Outdated
| @@ -3163,14 +3171,22 @@ def read_staticgeoms( | |||
| def write_geoms( | |||
| self, | |||
| geom_fn: str = "staticgeoms", | |||
| precision: int = 5, | |||
There was a problem hiding this comment.
I thought we said we would change default precision to 6?
There was a problem hiding this comment.
And maybe in the code, you could check is the mod.crs.is_projected and then adjust the default value to 1 for 0.1 meter? And document it in the docstring?
Which I now see should maybe be updated?
There was a problem hiding this comment.
Hehe, forgot that one. Good catch!
There was a problem hiding this comment.
Thanks for updating @dalmijn !
Two last remarks:
- the index name of self.geoms["basins"]
- the update of the examples. I agree that the geosjon should change with the new rounding precision but why inmaps. staticmaps and wflow_sbm files should change?
| @@ -3684,7 +3700,6 @@ def basins(self): | |||
| .set_index("value") | |||
| .sort_index() | |||
| ) | |||
| gdf.index.name = self._MAPS["basins"] | |||
There was a problem hiding this comment.
Which test did it break? Maybe actually using wflow_subcatch rather than value as name is better no? While we are breaking behavior of self.basins anyhow :)
Issue addressed
Fixes #173, #83
Explanation
Code.
Checklist
mainAdditional Notes (optional)
No.