add setup_river_floodplain method

[DirkEilander]

see #107

TODO:

• create tests
• add to docs
• add toml setting in setup_river_floodplain
• add options for manning roughness
• merge setup_hydrodem & setup_river_floodplain?
• release and update pyflwdir dependency.

[DirkEilander]

Note that setting a list via the hydromt ini file does not work currently. A fix is proposed in Deltares/hydromt#246

[DirkEilander]

@JoostBuitink @laurenebouaziz @verseve

There are a few last TODO's to finalize this PR. see description above. Regarding the question whether to merge setup_hydrodem and setup_river_floodplain, I'd like to get your opinion. Both methods deal with (different ways) to schematize the floodplains and to use the 1D floodplains you still need to run setup_hydrodem first. What is a clear way to divide the different processes?

My suggestion:

• remove setup_hydrodem
• move the setup of 1D riverbed elevation (hydrodem) to setup_rivers.
• move the setup of 2D elevation (i.e. updating the hydrodem) to setup_floodplains if the user chooses the 2D floodplain option.

Also, I cannot reproduce the failing test locally. Does it also fail for you? I'll try to do a clean checkout to see if there is a local file conflict somewhere..

[laurenebouaziz]

@DirkEilander, yes I think this is a very good suggestion! Now we have:

[input.lateral.river]
bankfull_elevation = "hydrodem_subgrid_D4"

but in fact in case we have floodplain 1d routing, D4 or D8 does not matter because the river is D8 anyways. So we could have this standard as D8 if floodplain 1d is applied.

If 1d2d is applied, it makes sense to update to D4 in a specific setup_2d_floodplains method (maybe good to have 2d included in the name of the method to make it very clear?)

@verseve, we were just discussing with @JoostBuitink that the toml also includes:
[input.vertical]
altitude = "wflow_dem"

how is this used in wflow and should this also be consistent (dem versus subgrid?)

About the test, I had updated the staticmaps and the local tests were working for me, but I will check again and add some more tests as we discussed!

[verseve]

@verseve, we were just discussing with @JoostBuitink that the toml also includes: [input.vertical] altitude = "wflow_dem"

how is this used in wflow and should this also be consistent (dem versus subgrid?)

This is not used by the sbm model type, it is only used by the sbm_gwf model type (as surface elevation).

[verseve]

but in fact in case we have floodplain 1d routing, D4 or D8 does not matter because the river is D8 anyways. So we could have this standard as D8 if floodplain 1d is applied.

Is it also then still possible to use the average dem value (hydrodem_avg ?) ?

[DirkEilander]

but in fact in case we have floodplain 1d routing, D4 or D8 does not matter because the river is D8 anyways. So we could have this standard as D8 if floodplain 1d is applied.

Is it also then still possible to use the average dem value (hydrodem_avg ?) ?

Yes, we will keep this option. Or perhaps even remove the other option (subgrid outlet pixel elevation) as it is not a logical approach as we discussed this largely overestimates the floodplain volume, right? For the river bed elevation I will then also implement #126

[laurenebouaziz]

but in fact in case we have floodplain 1d routing, D4 or D8 does not matter because the river is D8 anyways. So we could have this standard as D8 if floodplain 1d is applied.

Is it also then still possible to use the average dem value (hydrodem_avg ?) ?

Yes, we will keep this option. Or perhaps even remove the other option (subgrid outlet pixel elevation) as it is not a logical approach as we discussed this largely overestimates the floodplain volume, right? For the river bed elevation I will then also implement #126

I think the subgrid option is good to keep, because it has a more realistic river slope profile right? with the average option, there was too much delay in the Meuse and Rhine models.

[DirkEilander]

pyflwdir v0.5.7 has been released https://pypi.org/project/pyflwdir/

[JoostBuitink]

• add riv_uparea attribute to metadata of river map in staticmaps. Use this if no uparea is specified in the setup_floodplain function (raise error if different)
• check against pre-build model layers
• update toml with new pyflwdir version

[DirkEilander]

Really nice work @JoostBuitink
I made two small suggestion, but in general I think this is good to go.
I'm however not allowed to approve it as I started the PR.

[JoostBuitink]

It looks like the glacier map was failing the tests. I reverted the glacier map back (in 7f9909e) to the original map (see #143), as this now again represents expected behavior with all_touched=True

[JoostBuitink]

Reviewing on behalf of @DirkEilander, quoting:

Really nice work @JoostBuitink I made two small suggestion, but in general I think this is good to go. I'm however not allowed to approve it as I started the PR.

The two small suggestion are included, so this PR is good to go