States

[hboisgon]

Issue addressed

Fixes #18

Explanation

Added:

• read and write methods for states
• clip method for states
• prepare cold states method

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

• 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)

Add any additional notes or information that may be helpful.

[dalmijn]

@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)
• 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)
• Nodata value of waterlevel_lake
• 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.

[hboisgon]

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?

[dalmijn]

Thanks @hboisgon for addressing the comments!

Everything looks good except for one:

@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 :)

This is not handled properly in write_states. state.path_input will not be set.
An error will be thrown in write_states as there is no value for it in the config.

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?

I agree, I was more thinking of a added cell in the current update or build notebook, not a separate one.

[dalmijn]

@hboisgon, Thanks for the update :)!

I have one last point remaining.

• setting state.path_input

[dalmijn]

LGTM.

@hboisgon could you take a quick look at the make_path_relative functionm?

[hboisgon]

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]

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