search

Deltares / hydromt_wflow

Wflow plugin for HydroMT
https://deltares.github.io/hydromt_wflow/
GNU General Public License v3.0
15 stars 13 forks source link

States #252

Closed hboisgon closed 4 months ago

hboisgon commented 5 months ago

Issue addressed

Fixes #18

Explanation

Added:

To be precise in the cold states method as this can be done after a wflow model has been updated and calibrated, I added a new function to get the right values based on value/scale/offset options in the config file with a new utils.get_grid_from_config function.

Checklist

Additional Notes (optional)

Add any additional notes or information that may be helpful.

hboisgon commented 4 months ago

Thanks for the review @dalmijn ! I addressed most of comments. Could you check again? Some extra replies:

@hboisgon, Thanks for the good work! For the most part, it looks very solid!

I had a few points:

  • path_input is not set (could cause issues with custom config files)

This is handled in the read_states and write_states methods normally :)

  • Same argument goes for the states themselves ( I realise we rely upon the blueprint settings files, but this makes it a bit more future proof)

I added.

  • Nodata value of waterlevel_lake

Updated.

  • A bit more documentation maybe in the building (or updating) model notebooks that this can be done. I'd say it's really key that the states can be set like this.

The only use case for this I know is using wflow in Delft-FEWS because Wflow.jl can setup its own cold states so you don't want to prepare them by default (so should not be included in the wflow_build.yml template). The use is also not really complex so not sure if I need a specific update example for it. It's already documented in the list of available methods in the wflow docs and properly listed in the API. Do you think we really need to add an example?

hboisgon commented 4 months ago

Hi @dalmijn. In the end I preferred to handle this similarly to other components like forcing. While checking forcing and grid, I realized that wflow also supports dir_input as relative path to all input files including states so I added that as well. In the end I do not think we need the function to deal with relative path that you added (even if it was a good one to check for mount first). If you agree then we can remove it I think.

dalmijn commented 4 months ago

@hboisgon, It look good to me. I'll delete the excess functions.