feat: update config parameters and add upgrade to v1.1#765
Conversation
TODO add tests & toml files for upgrading v1 to v1.1
refactor current upgrade tests updated file structure for upgrade tests
There was a problem hiding this comment.
Pull request overview
This PR updates HydroMT-Wflow to support Wflow.jl v1.1 configuration changes and introduces an “upgrade to latest” upgrade path, along with updated reference configs/examples and expanded upgrade test coverage.
Changes:
- Added
wflow_versionto default config headers, templates, examples, and upgrade test fixtures; introducedWFLOW_LATEST_VERSION = 1.1. - Implemented v1.0 → v1.1 config conversion for SBM (
convert_to_wflow_v1_1_sbm) and addedupgrade_to_latest()flow for SBM and Sediment models. - Refactored/expanded upgrade tests and centralized upgrade test data under
tests/data/upgrade; updated docs and notebooks to referenceupgrade_to_latestandpixi run ...CLI usage.
Reviewed changes
Copilot reviewed 45 out of 66 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_version_upgrade.py | New upgrade test suite covering v0→v1 and v1→v1.1, plus end-to-end “latest” upgrades. |
| tests/test_utils.py | Removes legacy upgrade-related tests now covered in test_version_upgrade.py. |
| tests/data/upgrade/sediment/v1_1/wflow_sediment.toml | Adds v1.1 sediment reference config for upgrade tests. |
| tests/data/upgrade/sediment/v1_0/wflow_sediment.toml | Adds wflow_version=1.0 and updates path_static. |
| tests/data/upgrade/sediment/v0x/wflow_sediment.toml | Adds v0.x sediment input config fixture. |
| tests/data/upgrade/sbm/v1_1/wflow_sbm.toml | Updates SBM config to v1.1 expectations (incl. renamed state/static keys). |
| tests/data/upgrade/sbm/v1_0/wflow_sbm.toml | Adds wflow_version=1.0 and updates some paths/options. |
| tests/data/upgrade/sbm/v0x/wflow_sbm.toml | Updates v0.x SBM fixture structure (e.g., adds reservoirlocs, layered c). |
| tests/data/upgrade/readme.md | Documents the new tests/data/upgrade layout. |
| tests/components/test_config_component.py | Updates expectations to include wflow_version default/latest version. |
| pyproject.toml | Expands Ruff per-file ignores for tests. |
| pixi.lock | Lockfile hash update for the local package entry. |
| hydromt_wflow/workflows/states.py | Updates cold-state config key mapping for subsurface flow. |
| hydromt_wflow/wflow_sediment.py | Introduces upgrade_to_latest flow and internal upgrade steps. |
| hydromt_wflow/wflow_sbm.py | Introduces upgrade_to_latest, adds v1.1 upgrade step, refactors v0→v1 upgrade. |
| hydromt_wflow/version_upgrade.py | Adds WFLOW_LATEST_VERSION and SBM v1.0→v1.1 conversion helper. |
| hydromt_wflow/naming.py | Adds v1.1 key mappings for elevation and subsurface state variable names. |
| hydromt_wflow/data/wflow_sediment/wflow_sediment.toml | Sets default sediment template wflow_version=1.1. |
| hydromt_wflow/data/wflow_sbm/wflow_sbm.toml | Sets default SBM template wflow_version=1.1. |
| hydromt_wflow/data/default_config_headers.toml | Adds wflow_version=1.1 header default. |
| examples/wflow_update_v1_sediment.yml | Switches example update step to upgrade_to_latest. |
| examples/wflow_update_v1_sbm.yml | Switches example update step to upgrade_to_latest. |
| examples/wflow_sediment_piave_subbasin/wflow_sediment.toml | Adds wflow_version=1.1 to example config. |
| examples/wflow_piave_subbasin/wflow_sbm.toml | Adds wflow_version=1.1 to example config. |
| examples/wflow_piave_subbasin/wflow_sbm_results2.toml | Adds wflow_version=1.1 to example results config. |
| examples/wflow_piave_subbasin/wflow_sbm_results.toml | Adds wflow_version=1.1 to example results config. |
| examples/wflow_piave_clip/wflow_sbm.toml | Adds wflow_version=1.1 to clip example config. |
| examples/upgrade_to_wflow_v1.ipynb | Updates narrative/commands to upgrade_to_latest and pixi run. |
| examples/update_model_water_demand.ipynb | Normalizes notebook JSON + updates CLI commands to pixi run. |
| examples/update_model_landuse.ipynb | Normalizes notebook JSON + updates CLI commands to pixi run. |
| examples/update_model_gauges.ipynb | Normalizes notebook JSON + updates CLI commands to pixi run. |
| examples/update_model_forcing.ipynb | Updates CLI commands to pixi run. |
| examples/linux64/wflow_sediment_piave_subbasin/wflow_sediment.toml | Adds wflow_version=1.1 to linux64 example config. |
| examples/linux64/wflow_piave_subbasin/wflow_sbm.toml | Adds wflow_version=1.1 to linux64 example config. |
| examples/linux64/wflow_piave_subbasin/wflow_sbm_results2.toml | Adds wflow_version=1.1 to linux64 example results config. |
| examples/linux64/wflow_piave_subbasin/wflow_sbm_results.toml | Adds wflow_version=1.1 to linux64 example results config. |
| examples/linux64/wflow_piave_clip/wflow_sbm.toml | Adds wflow_version=1.1 to linux64 clip example config. |
| examples/clip_model.ipynb | Updates CLI command to pixi run. |
| examples/build_sediment.ipynb | Updates CLI commands to pixi run. |
| examples/build_model.ipynb | Updates CLI commands to pixi run and notebook metadata. |
| docs/user_guide/3_sediment_model/1_methods_components.rst | Updates docs to reference upgrade_to_latest. |
| docs/user_guide/2_sbm_model/1_methods_components.rst | Updates docs to reference upgrade_to_latest. |
| docs/getting_started/faq.rst | Updates FAQ upgrade references to upgrade_to_latest. |
| docs/changelog.rst | Adds v1.1 upgrade note and mentions rename to upgrade_to_latest. |
| docs/api/sediment-model.rst | Updates API docs to list upgrade_to_latest. |
| docs/api/sbm-model.rst | Updates API docs to list upgrade_to_latest. |
Comments suppressed due to low confidence (1)
hydromt_wflow/workflows/states.py:117
- prepare_cold_states() now always writes the v1.1 subsurface state key (subsurface_water__instantaneous_volume_flow_rate). That will generate an invalid/unknown key for v1.0 models (which still use subsurface_water__volume_flow_rate), breaking setup_cold_states on v1.0 configs. Please switch the key based on config['wflow_version'] (or default to v1.0 if absent/invalid).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r__instantaneous_volume_flow_rate' in all example config.tomls
deltamarnix
left a comment
deltamarnix
left a comment
There was a problem hiding this comment.
I know you're still working on this, but I already want to give some feedback on what I see happening.
| @@ -13,7 +13,7 @@ path_output = "outstate/outstates.nc" | |||
|
|
|||
| [input] | |||
| path_forcing = "inmaps.nc" | |||
| path_static = "staticmaps/staticmaps_v0x.nc" | |||
There was a problem hiding this comment.
I didn't expect this file to be upgraded, since it is a specific v0x file.
There was a problem hiding this comment.
I changed the file structure to have the model types (sbm/sediment) and versions (v0x/v1_0/v1_1) be the directories, then each model has the default names for the files.
Each model dir has its own staticmaps & config.toml that we can use in tests for assertions.
previous impl was difficult to extend nicely for more versions.
There was a problem hiding this comment.
That sounds good, but why are there some new variables in this file? reservoirlocs, input.vertical.c, csv.column
There was a problem hiding this comment.
The previous version tested v0 without lakes and v0 with lakes separately, which seems redundant to me, so I merged them. Now we test with one v0 model that has lakes.
The changes are the lakes that I added from the other v0x.
See tests/data/wflow_v0x/sbm_with_lake_files/wflow_sbm_v0x.toml
|
Just talked to @JoostBuitink about this, and there is a decision required form the wflow.jl team about the config parameters. He will discuss with the team and come back here with the names of the newly added required config parameters. |
Note that it should be |
implement Marnix's review comments
Why the second param is needed, is a question for @JoostBuitink Why we use different names: The mappings are defined in naming.py. For this parameter:
Most setup methods call So, even though hydromt_wflow internally uses |
Thanks for the explanation Luuk! I indeed thought this was about Wflow.jl parameter names. Briefly discussed this today with Joost, and the second parameter is required: |
…DIR is the actual global var, the rest of the codebase uses it.
|
I am fairly confident that something goes wrong in the 2D floodplains function: as @vers-w mentioned, there is a D4-conditioned DEM that should be set to the This also means that an update to the naming dictionary is preferred, see my proposal below (also tagging @vers-w for his opinion). Please note the comments I put in this as well # NOTE: this was "land_elevation" before
"land_elevation_D4": {
"wflow_v0": "lateral.land.elevation",
"wflow_v1": "land_surface_water_flow__ground_elevation",
"hydromt_name": "hydrodem_D4",
},
"land_elevation": {
"wflow_v0": "altitude", # NOTE: old (sbm_gwf) models already had this key, although it was stored under [input], and not under [input.vertical] or [input.lateral] like most other keys. So please check if this works
"wflow_v1": "land_surface__elevation", # this was actually already a field in v1.0, however, it now becomes a hard requirement in v1.1
"hydromt_name": "elevtn",
},For getting the "land_surface__elevation", I think this dictionary needs to be updated hydromt_wflow/hydromt_wflow/wflow_base.py Lines 297 to 301 in ac1dd87 and note these two sections as well (right not elevation is explicitly skipped, this is no longer needed) hydromt_wflow/hydromt_wflow/wflow_base.py Lines 443 to 446 in ac1dd87 hydromt_wflow/hydromt_wflow/wflow_base.py Lines 460 to 463 in ac1dd87 |
Decided to directly tackle it in this PR, perhaps easier to do it in one go with the expected change in the naming dictionary as well. I guess that the tests will fail as it will produce a slightly different toml, so that would need a small fix |
rebuild example models
|
|
||
| Example yml: | ||
| ``` | ||
| 0.8_1.0: |
There was a problem hiding this comment.
The "from" might not be bothered by the user, because it is always "previous_to_X.X". I would suggest to name them: "to_1.0" and "to_1.1". Or just the version number it will upgrade to. "1.0" and "1.1".
There was a problem hiding this comment.
I like being explicit here. I think it communicates to users exactly what we expect and forces users to understand the state and version of their model.
to_1.1 or 1.1 could be misinterpreted, for either the whole upgrade chain up until 1.1, or 1.0 to 1.1, or even 1.1 to 1.2 in the future.
…ntime code for mapping wflow.jl variables to layers in staticmaps. version_upgrade.py no longer uses naming.py and keeps a separate list of variables used for upgrading existing models.
deltamarnix
left a comment
deltamarnix
left a comment
There was a problem hiding this comment.
I haven't checked the whole migration, but I have the idea that changing the config itself can be simplified by having a starting point, instead of rebuilding it from the ground up.
| @@ -13,7 +13,7 @@ path_output = "outstate/outstates.nc" | |||
|
|
|||
| [input] | |||
| path_forcing = "inmaps.nc" | |||
| path_static = "staticmaps/staticmaps_v0x.nc" | |||
There was a problem hiding this comment.
That sounds good, but why are there some new variables in this file? reservoirlocs, input.vertical.c, csv.column
| land_routing = "kinematic_wave" | ||
| river_routing = "kinematic_wave" | ||
| kinematic_wave__adaptive_time_step_flag = false | ||
| kinematic_wave__adaptive_time_step_flag = true |
There was a problem hiding this comment.
Why did this change from false to true?
There was a problem hiding this comment.
In the removed test test_utils.py::test_convert_to_wflow_v1_sbm_with_exceptions, this was set to false to make the assert pass, which is also weird imo.
| reservoir_water__target_full_volume_fraction = "reservoir_target_full_fraction" | ||
| reservoir_water__target_min_volume_fraction = "reservoir_target_min_fraction" | ||
|
|
||
| [input.static.soil_layer_water__brooks_corey_exponent] |
There was a problem hiding this comment.
Same as other files. Why did the configs change?
There was a problem hiding this comment.
These changes are due to the merge of the previous v0 models in examples/data/wflow_upgrade (with / without lakes).
The merged model is at tests/data/upgrades/sbm/v0x
Which was upgraded and stored in tests/data/upgrades/sbm/v1_0
Which was upgraded and stored in tests/data/upgrades/sbm/v1_1
So changes in the v0 model is the culprit.
There was a problem hiding this comment.
Also note that git doesnt show these additions to the file tests/data/upgrade/sbm/v1_0/wflow_sbm.toml.
That is because they were already there before in the sbm_with_lakes v0 model.
| # v1.1+ | ||
| if "wflow_version" in config: | ||
| logger.debug("Found wflow_version, indicating v1 schema.") | ||
| return True |
There was a problem hiding this comment.
I'm not sure if I understand this function. The description says that it detects a v1.0 config schema, but it returns true if a "wflow_version" is present, which means it's actually a 1.1 schema. So why does it return true?
There was a problem hiding this comment.
good catch. removed it now.
_is_v1_schema was only called by _detect_version_from_config, which checks the config for the wflow_version variable before calling _is_v1_schema, so this would not have bugged, but still good to be clear about intent.
def _detect_version_from_config(config: dict) -> Version:
"""Detect the Wflow.jl version from a raw config dictionary.
Parameters
----------
config : dict
The raw TOML config dictionary.
Returns
-------
Version
The detected Wflow.jl version.
"""
version = get_config(config=config, key="wflow_version", fallback=None)
if version is not None:
logger.debug(f"Detected Wflow version {version} from config.")
return Version(str(version))
if _is_v1_schema(config):
logger.debug(
"No 'wflow_version' found in config but v1.0 schema detected. "
"Assuming v1.0 model."
)
return Version("1.0")
logger.warning(
"No 'wflow_version' found in config and no v1.0-only keys detected. "
"Assuming pre-v1.0 model with v0.8 schema. Please check the config and update "
"the version if needed."
)
return Version("0.8")| # v1 output structure | ||
| output_cfg = config.get("output", {}) | ||
|
|
||
| if "netcdf_grid" in output_cfg or "netcdf_scalar" in output_cfg: |
There was a problem hiding this comment.
If we get all the way here, we expect that all the other stuff was not present, or it was still in v0 format. How do you check that difference? If it is a Frankenstein version, I'm not sure if we should be returning true by now.
There was a problem hiding this comment.
Many of these checks are optional features for v1 that we upgrade manually in _convert_to_wflow_v1.
So they might not all be present in user models at the same time.
To make sure we dont mis categorize, I think we should try to check and catch them all.
I agree with you, its not likely we ever get to this last return True statement, but that is just an order thing.
If we change the order, do we then remove a different one?
| # Initialize the output config | ||
| logger.info("Converting config to Wflow v1 format") | ||
| logger.info("Converting config general, time and model sections") | ||
| config_out = {} |
There was a problem hiding this comment.
Would it be possible to start with the original config and move/remove the parts that have changed? It looks like we have to go over all parts of the config, but actually most of the config is in the right shape. There are only changes to take care of.
There was a problem hiding this comment.
For v0 to v1, I think starting from scratch is much simpler tbh.
The structure of the toml file significantly changed, variables were moved, renamed etc.
Any config key we dont explicitly support in the version upgrade, is dropped, ensuring we can never accidentally forget to delete a v0 key, silently creating a mixed v0/v1 config.
For v1 to v1.1, the patch-style upgrade is already in place.
| continue | ||
| name = get_config(key=f"state.{v0_var}", config=config, fallback=None) | ||
| if name is not None: | ||
| config_out["state"]["variables"][v1_var] = name |
There was a problem hiding this comment.
Would it be easier to use a toml library that can directly handle variables with periods (.) in the names? Now I see we have to build up a bunch of dictionary entries with empty statements, while we actually just want to move things around.
There was a problem hiding this comment.
toml libraries are for reading/writing to disk.
get_config and set_config work on already parsed toml files: in-memory dicts.
so Im not sure what we would gain by doing that.
|
Issue addressed
Fixes #727
Explanation
pixi runto all cli commands in examples to get them to run (how did this work before? would like to revert these changes)wflow_versionattribute.tests/data/upgradehas all data for testing.examples/data/wflow_upgradehas the files for the upgrade example.)Files Changed
There are many many files changed, but not that many significant ones. Most changes are updating the example models / other data in the repo.
For reviewers, these are the changes from most relevant/in need of review, to least relevant / interesting:
version_upgrade.py & wflow_sbm.py & wflow_sediment.py: new system to upgrade to latest versiontests/test_version_upgrade.py: tests for the upgrade system (isolated tests for each upgrade step, aggregate test for v0 -> latest as well)tests/data/upgrade: organized models + data for testing the upgrades (no longer uses example models.).github/workflows/system_test.yml: now we useWflow.jl@masterinstead ofWflow.jl@release/v1.0. TODO!hydromt_wflow/workflows & naming.py: config parameter renames according to the issue. `hydromt_wflow/data: added wflow_version = "1.1" to default configtests/components: updates for config component, "wflow_version" now in config.examples/*.ipynb: updates to cli commands that were broken.examples/: rebuilt example models on windows and linux, updated config tomls.docs/: renamedupgrade_to_wflow_v1toupgrade_to_latestTODO
release/v1.1in wflow.jl once it exists