Parse TOML to a nested struct by SouthEndMusic · Pull Request #689 · Deltares/Wflow.jl

SouthEndMusic · 2025-09-08T05:49:01Z

Fixes #243
Fixes #688

The original idea was to use Configuration.jl to parse the TOML, but after discussion with @JoostBuitink and @vers-w we decided not to do this. Instead the implementation is custom, using AbstractConfigSection and a parse function init_config_section.

An overview of the changes made in this PR:

The package PropertyDicts is now used to access nested dictionaries as if they were nested structs (not sure if this is still actually required though)
Conversion to the proper type of a config field is done in init_config_section, so the types do not have to be specified anymore in the code where the config field is used
The package EnumX is used to make enumerations of config options, see the top of config_structure.jl. These enumerations are also used to validate these config options
Some work was done to write the Config object back to TOML with the function to_dict, which is a requirement for Write the parsed config to output #701
Basic printing functionality was added for the Config object mainly for developers, but this could be added to the log if desirable
Some constructors were removed that are now automatically generated with the @with_kw macro
Some function names were changed to reflect that they are constructors of a particular type
Some function signatures were made more specific for clarity

vers-w

Nice work @SouthEndMusic !

Besides a couple of comments/questions, the code coverage has decreased and is failing, also at the project level, would be good to bump this up. I think it would be good if @JoostBuitink also checks the config settings in config.jl.

vers-w · 2025-09-09T06:43:36Z

+    reservoir_area__count::String = ""
+    reservoir_location__count::String = ""


If reservoir__flag is true these are required, maybe worth adding?

You mean as a comment here? I don't think Configurations.jl supports validation logic like 'if this condition for this field, then that condition for that field'. I think it's best if such conditional validation is done in the code where the field is parsed.

OK, yeah, it seems indeed Configurations.j does not support this kind of validation. But it would be nice to add this validation to the code, if reservoir__flag is true, reservoir_area__count and reservoir_location__count should be set (not default empty string).

OK, I'll set the default to nothing and then raise an error when the data is required.

@vers-w can you point to the best place in the code to validate this? Probably the earliest relevant point in the initialization

I think it would be best to validate this already when reading the config. Given that Configurations.jl does not support this, and appears to be not too actively maintained, I wonder if it is worth writing our own logic around this, or whether it is better to define our own config object.

It would also be nice to check if certain layers required specifically to the model type/settings (for example layers like soil moisture, glacier locations, reservoir parameters, floodplain volumes) are present. It is a bit tricky that those can be defined both in the [input.static] or [input.cyclic] settings.

Also, some fields like river_routing only support two options ("kinematic-wave" or "local-inertial"). If a user tries something else (or a typo), it would be best to throw an error when creating the config object (rather than within the computation code/initialization like it is now). I once tried to get this sorted using Configurations, but failed to do so.

Yes, I would say as part of the function Config?

This hints at a difference we have in 'config philosophy'. In Ribasim the config object is a static representation of the TOML which is only used in initialization (where any validation is done) and post-processing. I'm fine with doing this differently in Wflow, as long as the approach is consistent. I'll look into moving as much of the config validation to the Config constructor as possible.

Not sure if I understand your comment completely, but generating the config object is the first step before initializing a model from that config. So, I think it makes sense to do the validation here, as @JoostBuitink also indicated.

vers-w · 2025-09-09T08:34:29Z

+end
+
+# Model configurations
+@option struct ModelSection <: AbstractConfigSection


Maybe worth to have model sections per model type?

That would be nice, but then we have to either:

Also require this in the TOML, which is breaking

Do some more processing of the TOML read before passing it to Configurations.jl, which sort of defeats the purpose of Configurations.jl

SouthEndMusic

/

SouthEndMusic · 2025-09-11T20:31:51Z

+    river = config.model.river_routing
+    @assert river ∈ ROUTING_OPTIONS  # Already validated in `validate_config`

    subsurface = config.model.type == "sbm" ? "kinematic-wave" : "groundwaterflow"


The nothing here used to be "groundwaterflow" but that is referenced nowhere else

What nothing do you mean here?

SouthEndMusic · 2025-09-12T06:36:15Z

+# Logging related configurations
+@option struct LoggingSection <: AbstractConfigSection
+    silent::Bool = false
+    loglevel::String = "info"


Is the loglevel input allowed to be an integer?

From checking the code in logging.jl (and the docs) this is allowed.

SouthEndMusic · 2025-09-12T11:38:36Z

+                    var,
                    location_dim = nc_var["location"],
                    locations = [nc_var["location"]],
                ),


I feel like there is some validation missing here, or is that done somewhere else? That could already be done in config.jl

This is not vaildated somewhere else. What kind of validation were you thinking of?

SouthEndMusic · 2025-09-12T18:42:28Z

    if haskey(var, "layer")
-        v = get(config.model, "soil_layer__thickness", [100, 300, 800])::Vector{Int64}
-        inds = collect(1:(length(v) + 1))
+        inds = collect(1:(length(config.model.soil_layer__thickness) + 1))


I don't understand this code. What is the difference between dim_value and index (apart from the potential out of bounds error)?

Yeah, I agree, this function is not adding much. Probably a remainder of refactoring (another model concept did support also an extra_dim).

vers-w · 2025-09-15T06:31:43Z

As discussed we want to drop the Configurations.jl dependency, it appears to be not too actively maintained and more importantly it does not very well fit with the very flexible Wflow TOML (required workarounds). As we do not have that many mandatory fields, we can implement our own constructor methods (not relying on another dependency). More config validation can be done as part of another PR.

SouthEndMusic · 2025-09-16T11:34:55Z

After discussing with @evetion I'm motivated to keep Configurations.jl as a dependency, because:

The only part of the configuration that has both standard and flexible fields is the input section. This is handled in a simple wrapper of the default Configurations.from_dict for this section.
Other parts fit nicely in a schema as defined by Configurations.jl, including the very flexible output section. This means that a lot of parsing of the entries here can be done (in a simpler way) in config.jl than in io.jl.
In general, overloading of Configurations.from_dict avoids all needs to work around Configurations.jl as I did before. This is within the documented usage of Configurations.jl, specifically this part.
A particularly nice conversion that we can let Configurations.jl do this way is from string options to enumerator (EnumX.Enum) instances. This means that we have a centralized way to define which options a particular part of the configuration has and to throw an informative error if an invalid option was given in the TOML.

My apologies for making the PR so big. I do believe that (most of) this is a good step in making the code easier to understand as a newcomer

vers-w · 2025-09-23T10:55:12Z

One thing that I think is worth reviewing is what the info messages now look like. The order has changed, and some things are also formatted differently.

I have checked the formatting of the log messages with sbm_config.toml run. Some message seem a bit too verbose, for example:

┌ Info: Set `subbasin_location__count` using netCDF variable `
│ 		scale	= [1.0]
│ 		offset	= [0.0]
│ 		external_name	= wflow_subcatch
└ `.

previously:

[ Info: Set `subbasin_location__count` using netCDF variable `wflow_subcatch`.

Do we need to add these default scale and offset fields?

For the scalar output the formatting can be improved a bit I think (is the field _was_specified useful?):

┌ Info: Adding scalar output for linear index.
│   fileformat = "CSV"
│   param = "reservoir_water__volume"
│   index =
│    
│    		i	= 1
│    		_was_specified	= true
└

And it seems something goes wrong with the reducer only, now listed as :nothing, for example:

┌ Info: Adding scalar output for a map with a reducer function.
│   fileformat = "CSV"
│   param = "river_water__volume_flow_rate"
│   mapname = "river_gauge__count"
└   reducer_name = :nothing

JoostBuitink · 2025-09-23T15:14:26Z

In addition:

the layers that are scaled with a scale and/or offset are now logged before the actual Wflow.jl logging. Would be nicer to log this later, similar to how it was before (also related to Willem's comment).
when running a model without a dir_input, I get this error: ERROR: MethodError: no method matching normpath(::String, ::Nothing, ::String). This is because dir_input is default set to nothing, while it should be "." I think. The same can be done for the dir_output, right?

vers-w · 2025-09-25T08:57:28Z

Just did a final check of the logging messages:

Not sure if it is required to log when a parameters is modified with scale 1.0 and offset 0.0:
Info: NetCDF parameter wflow_ldd is modified with scale 1.0 and offset 0.0.
I think it is better to remove these, @JoostBuitink ?
It would be nice to use single quotes for the parameter names and values (this is clearer in the console) and already used.
Something goes wrong with formatting this external parameter name?:
[ Info: NetCDF parameter wflowgaugesgrdc is modified with scale 1.0 and offset 0.0.

vers-w

LGTM! A couple of small comments about the logging #689 (comment).

SouthEndMusic · 2025-09-25T09:58:16Z

@vers-w I resolved all the merge conflicts but there is still one test failing that I don't know how to fix. Can you have a look?

Caused by fixing `snow_gravitational_transport__flag` TOML key.

vers-w · 2025-09-25T20:08:37Z

@vers-w I resolved all the merge conflicts but there is still one test failing that I don't know how to fix. Can you have a look?

@SouthEndMusic : I have resolved the tests and changed the log message for modifying NetCDF parameters (5bcfa41). I think it is fine to merge, @JoostBuitink: maybe you want to do a final check?

JoostBuitink · 2025-09-26T07:20:38Z

@vers-w I resolved all the merge conflicts but there is still one test failing that I don't know how to fix. Can you have a look?

@SouthEndMusic : I have resolved the tests and changed the log message for modifying NetCDF parameters (5bcfa41). I think it is fine to merge, @JoostBuitink: maybe you want to do a final check?

A did a very quick check, but the error when trying to run a simulation without a dir_input defined is still present. Note the Wflow.Config() works fine in this case, but Wflow.run() breaks (see error in my earlier comment). All our test tomls contain a dir_input, so perhaps something we should cover in the tests. I assume the same holds for the case when no dir_output is specified.

I'll try to do a slightly more detailed review and testing later today.

Also removed optional toml/config path (nothing).

vers-w · 2025-09-26T09:32:29Z

A did a very quick check, but the error when trying to run a simulation without a dir_input defined is still present. Note the Wflow.Config() works fine in this case, but Wflow.run() breaks (see error in my earlier comment).

Commit 2dbd6a5 should resolve this. I also did change the type of field path of Config to String, not clear why Union{Nothing, String} is required? It also seems not part of any test.

JoostBuitink

Thanks for the quick fix @vers-w! I did a (quick) review and some testing, and all seems to work! So we are good to merge I think!

vers-w · 2025-09-26T15:23:08Z

Thanks for the quick fix @vers-w! I did a (quick) review and some testing, and all seems to work! So we are good to merge I think!

Yes, @SouthEndMusic : if you agree with the code changes in 2dbd6a5 (including changing Config.input type), we are good to merge!

SouthEndMusic added 8 commits September 7, 2025 21:22

Add config.jl + code cleanup

e4c74da

Don't change julia version in Manifest

368a320

Merge branch 'master' into toml_parse

7a96e04

Update server

e9a4230

Restore server Project.toml

0f34ea0

merge fix

a5caf52

Another merge fix

777a66b

Update docs env

6da0858

SouthEndMusic requested a review from vers-w September 8, 2025 07:50

vers-w requested changes Sep 9, 2025

View reviewed changes

vers-w reviewed Sep 9, 2025

View reviewed changes

Comments addressed

0f2e1ee

SouthEndMusic commented Sep 10, 2025

View reviewed changes

SouthEndMusic added 6 commits September 10, 2025 13:03

Improve test coverage

e626f2f

Ignore extra TOML fields

493ab20

Do it right this time

8c64e74

Bring back river_gauge_* for now

f04db8e

Move flexible part of input section to config.input.flexible

1b51a4e

Make cold_start__flag mutable

e6f288a

SouthEndMusic requested a review from vers-w September 10, 2025 15:01

Move config option validation to config.jl

d092f4c

SouthEndMusic commented Sep 11, 2025

View reviewed changes

SouthEndMusic commented Sep 12, 2025

View reviewed changes

A lot of stuff

7059e91

SouthEndMusic added 2 commits September 16, 2025 14:01

Fix Manifest

5a60c04

run CI RUN

2a6d305

Merge branch 'master' into toml_parse

b4a4a9d

SouthEndMusic mentioned this pull request Sep 18, 2025

Write the parsed config to output #701

Closed

Merge master

2e2eb85

SouthEndMusic changed the title ~~Parse TOML with Configurations.jl~~ Parse TOML to a nested struct Sep 18, 2025

SouthEndMusic added 2 commits September 24, 2025 13:55

Merge branch 'master' into toml_parse

683ca47

Comments addressed

ef7343b

JoostBuitink mentioned this pull request Sep 25, 2025

Get rid of nested "netcdf.variable.name" entry #699

Closed

vers-w approved these changes Sep 25, 2025

View reviewed changes

Merge branch 'master' into toml_parse

9356f9b

SouthEndMusic and others added 7 commits September 25, 2025 12:55

Only print scale and offset if they are not default

72d0ecd

Add backticks to names and values in info message

0206dbe

Merge branch 'master' into toml_parse

433ff5f

Fix typo toml file

6a277bd

Update test value

8625071

Caused by fixing `snow_gravitational_transport__flag` TOML key.

Set logging to silent in io.jl test

9e8fec1

Log message modifying NetCDF parameter

5bcfa41

Fix default dir_input and dir_output

2dbd6a5

Also removed optional toml/config path (nothing).

JoostBuitink approved these changes Sep 26, 2025

View reviewed changes

JoostBuitink mentioned this pull request Sep 26, 2025

Fix/netcdf variable name #709

Merged

5 tasks

Merge branch 'master' into toml_parse

f73e807

SouthEndMusic merged commit 484d2bd into master Sep 29, 2025
7 of 9 checks passed

SouthEndMusic deleted the toml_parse branch September 29, 2025 09:31

		reservoir_area__count::String = ""
		reservoir_location__count::String = ""

Uh oh!

Conversation

SouthEndMusic commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vers-w left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SouthEndMusic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vers-w commented Sep 15, 2025

Uh oh!

SouthEndMusic commented Sep 16, 2025

Uh oh!

vers-w commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoostBuitink commented Sep 23, 2025

Uh oh!

vers-w commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vers-w left a comment

Choose a reason for hiding this comment

Uh oh!

SouthEndMusic commented Sep 25, 2025

Uh oh!

vers-w commented Sep 25, 2025

Uh oh!

JoostBuitink commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SouthEndMusic commented Sep 8, 2025 •

edited

Loading

vers-w commented Sep 23, 2025 •

edited

Loading

vers-w commented Sep 25, 2025 •

edited

Loading

JoostBuitink commented Sep 26, 2025 •

edited

Loading