Real wflow basins

[dalmijn]

Issue addressed

Fixes #173, #83

Explanation

Code.

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)

No.

[hboisgon]

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.

[hboisgon]

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.

[hboisgon]

I thought we said we would change default precision to 6?

[hboisgon]

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?

[dalmijn]

Hehe, forgot that one. Good catch!

[dalmijn]

@hboisgon, I do think the model res basins are better for consistency, but I do agree with you that for now (until #262 is fixed) that the highres basins are then better as a temporary fix. I'll change it back. Good thinking.

[hboisgon]

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?

[hboisgon]

Thanks for the adjustments @dalmijn
I approve already but I saw one last thing that you changed:

• if crs is projected, users cannot change the precision in write_geoms

Can you still update before merging? While you are at it, also add the change in precision of the geoms file to the changelog.

[hboisgon]

You added this since my last review no? Here the user cannot change that value at all the way you now coded it and for models like dflow fm precision of 10cm may not be good enough. Maybe you could instead set precision to None and then if precision is None and crs is projected then precision is 1 (maybe 2 is better?) and else 6? Then the user is fully in control again.

[dalmijn]

It was after your first review, we had a call concerning the precision when talking about degrees (which need quite some precision) and linear units (metres) which need less (when talking about wflow). I do agree that having it fixed is not ideal. I'll fiddle around with it a bit to make sure users can set it.