Refactor `SBM` concept #487

verseve · 2024-10-17T10:53:35Z

Issue addressed

Fixes #486

Explanation

See issue #486. Additionally, the vertical concepts HBV and FLEXTopo have been removed from the code, see also Discussion #342.

Checklist

Updated tests or added new tests
Branch is up to date with master
Tests & pre-commit hooks pass
Updated documentation if needed
Updated changelog.md if needed

* Remove vertical concept `FLEXTopo` * Remove vertical concept `HBV` * Update download test data for build * Cleanup docs Removed `HBV` and `FLEXTopo` concepts. * Removed code related to `FLEXTopo` Use of extra dim `classes`. * Remove and change river and land `inwater` functions As a result of removing `HBV` and `FLEXTopo` concepts. * Update changelog * Fix typo in docs Co-authored-by: JoostBuitink <[email protected]> --------- Co-authored-by: JoostBuitink <[email protected]>

* Cleanup metadata structs Remove `exchange`, `grid_type` and `grid_location` from metadata structs. `grid_type` is not required, it is already implemented as part of BMI. `exhange` and `grid_location` are now implemented without the use of the FieldMetadata package. * Update changelog * Fix `exchange` function for `ShallowWaterLand` * Add tests * Move `exchange` and `grid_location` functions To file bmi.jl as these functions are quite specific to BMI functionality. * Add `v1` branch to CI

* Add configuration file for Julia Formatter * Add format on save in vscode settings file * Remove .vscode from gitignore * Format all Julia files * Sync wflow's style guide with ribasim's style guide * Update formatting * Update .vscode/settings.json * Remove outdated comments * Add workaround for VS Code bugs

* Stop using local JULIAUP_DEPOT_PATH * Avoid error when override is already set Co-authored-by: Martijn Visser <[email protected]>

In `update` functions use `model` for interception, snow and glacier models. Add wrapper methods for snow runoff and glacier melt.

This is with kinematic wave routing. Renamed the model for `SBM` from `soil` to `bucket` as it is more related to this type of model (Simple Bucket Model) than a soil model that for example solves the Richards equation for the unsaturated zone and upper part of the saturated zone.

Also removed the vertical_process.jl file.

Mostly water demand and allocation functionality. All tests pass.

Add water demand and allocation, and snow transport computations.

Add separate functions to update the boundary conditions of the snow and `SBM` models.

In addition: - add a parameter set for LandParameters (to share accross model components). - make use of type NamedTuple (for external models) in the update of models and boundary conditions, these external models provide input to the models and boundary conditions. - move shared parameters (vegetation and land) to file Parameters.jl.

In update of models and boundaries of `LandHydrologySBM`

It only contains two parameters that are not shared that much across different models of `LandHydrologySBM`.

Use the struct name for outer constructor functions.

SouthEndMusic

I cannot comment on functionality yet, but I made some code quality suggestions

src/demand/water_demand.jl

src/utils.jl

SouthEndMusic · 2024-10-26T19:05:32Z

src/utils.jl

+    for i in eachindex(ssf)
+        ssfmax[i] = ((kh_0[i] * slope[i]) / f[i]) * (1.0 - exp(-f[i] * soilthickness[i]))
+        ssf[i] =
+            ((kh_0[i] * slope[i]) / f[i]) *
+            (exp(-f[i] * zi[i]) - exp(-f[i] * soilthickness[i])) *
+            dw[i]
+    end


Looks like this can be written without an explicit loop

Yes, good catch!

SouthEndMusic · 2024-10-26T19:06:26Z

src/vegetation/canopy.jl

+"Initialize interception model variables"
+function InterceptionVariables(
+    n;
+    canopy_potevap::Vector{T} = fill(mv, n),
+    interception_rate::Vector{T} = fill(mv, n),
+    canopy_storage::Vector{T} = fill(0.0, n),
+    stemflow::Vector{T} = fill(mv, n),
+    throughfall::Vector{T} = fill(mv, n),
+) where {T}
+    return InterceptionVariables(;
+        canopy_potevap = canopy_potevap,
+        interception_rate = interception_rate,
+        canopy_storage = canopy_storage,
+        stemflow = stemflow,
+        throughfall = throughfall,
+    )
+end


Can be done with default values as mentioned before

Make now use of a simple constructor as in water_demand,jl.

Co-authored-by: Bart de Koning <[email protected]>

Simplify external constructors.

Rewrite without explicit loop.

JoostBuitink

Very nice work, makes the code a lot more futureproof! I have a couple of improvements/questions, see the comments for those!

src/demand/water_demand.jl

src/io.jl

JoostBuitink · 2024-10-28T13:02:11Z

src/parameters.jl

+        sl = ncread(
+            nc,
+            config,
+            "vertical.vegetation_parameter_set.specific_leaf";
+            optional = false,
+            sel = inds,
+            type = Float,
+        )
+        swood = ncread(
+            nc,
+            config,
+            "vertical.vegetation_parameter_set.storage_wood";
+            optional = false,
+            sel = inds,
+            type = Float,
+        )


Would it be an idea to actually match the names of these parameter to what we set in the toml now as well? So sl would become specific_leaf (or maybe even storage_specific_leaf to be in line with storage_wood?), and swood would become storage_wood

I agree, was planning to actually look into this for all variables in Wflow as part of another PR. Anyway, good to already change these parameter names!

JoostBuitink · 2024-10-28T13:20:33Z

src/sbm.jl

+    if modelsnow && modelglacier
+        glacier_bc = SnowStateBC{Float}(; snow_storage = snow_model.variables.snow_storage)
+        glacier_model = GlacierHbvModel(nc, config, inds, dt, glacier_bc)
    else


Maybe good to throw a warning when modelglacier=true && modelsnow=false. It does not make sense to use this combination, but at least there will be some feedback to the user that glaciers will not be accounted for.

I agree and added a warning.

JoostBuitink · 2024-10-28T13:23:06Z

src/sbm_model.jl

    cellength = abs(mean(diff(x_nc)))

    sizeinmetres = get(config.model, "sizeinmetres", false)::Bool
    xl, yl = cell_lengths(y, cellength, sizeinmetres)
    riverfrac = river_fraction(river, riverlength, riverwidth, xl, yl)

+    lsm = LandHydrologySBM(nc, config, riverfrac, inds)


in sbm.jl this is called land_hydrology_model, so to be consistent, maybe change this to lhm?

Fair point, changed to lhm.

JoostBuitink · 2024-10-28T13:42:40Z

src/soil/soil.jl

+    if haskey(config.input.vertical.soil.parameters, "et_reftopot")
+        @warn string(
+            "The `et_reftopot` key in `[input.vertical.soil.parameters]` is now called ",
+            "`kc`. Please update your TOML file.",
+        )
+    end


For v1.0, I think we can get rid of this warning, right?

Yes, removed the warning.

JoostBuitink · 2024-10-28T15:56:29Z

src/vegetation/canopy.jl

+    e_r = ncread(
+        nc,
+        config,
+        "vertical.interception.parameters.eoverr";
+        sel = inds,
+        defaults = 0.1,
+        type = Float,
+    )


Best to match the names of the parameters here (either e_r or eoverr)

Good point, changed it to e_r.

test/run_sbm_piave.jl

-Use consistently `!` for functions that are mutating, and return always `nothing` for these functions. - Fix missing `canonicalize` from `Dates`

Co-authored-by: JoostBuitink <[email protected]>

…low.jl into refactor/sbm_concept

As of Julia 1.7 there is no difference and `isnothing` is also used in other code sections.

SouthEndMusic

As far as I can tell these changes look good. In Ribasim we don't have the mandatory return at the end of each function, but that is a matter of taste.

As briefly disscussed with @verseve, I am in favor of splitting the parameters and the states (i.e. the unknowns you are solving for) in your data structure, but let's leave that as a follow up. The main reason for splitting those is that it is more in alignment with the SciML standard.

SouthEndMusic · 2024-11-04T08:26:09Z

src/demand/water_demand.jl

+"Struct to store water demand model variables"
+@get_units @grid_loc @with_kw struct DemandVariables{T}
+    irri_demand_gross::Vector{T}        # irrigation gross demand [mm Δt⁻¹]
+    nonirri_demand_gross::Vector{T}     # non-irrigation gross demand [mm Δt⁻¹]
+    total_gross_demand::Vector{T}       # total gross demand [mm Δt⁻¹]
+    surfacewater_demand::Vector{T}      # demand from surface water [mm Δt⁻¹]
+    groundwater_demand::Vector{T}       # demand from groundwater [mm Δt⁻¹]
+end
+
+"Initialize water demand variables"
+function DemandVariables(T::Type{<:AbstractFloat}, n::Int)
+    return DemandVariables{T}(;
+        irri_demand_gross = zeros(T, n),
+        nonirri_demand_gross = zeros(T, n),
+        total_gross_demand = zeros(T, n),
+        surfacewater_demand = zeros(T, n),
+        groundwater_demand = zeros(T, n),
+    )
+end


I still think things like this can be squashed into one by using default field values allowed by @with_kw, or is there a specific reason you're not doing that?

Yes, not a specific reason, I would say it is a matter of taste.

SouthEndMusic · 2024-11-04T08:29:49Z

src/demand/water_demand.jl

+    inds_river = network.river.indices_allocation_areas
+    inds_land = network.land.indices_allocation_areas
+    res_index = network.river.reservoir_index
+    lake_index = network.river.lake_index


Suggested change

inds_river = network.river.indices_allocation_areas

inds_land = network.land.indices_allocation_areas

res_index = network.river.reservoir_index

lake_index = network.river.lake_index

(; indices_allocation_area, resevoir_index, lake_index) = network.river

inds_land = network.land.indices_allocation_areas

The only disadvantage of this is that you have to use the exact name of the field

Yes, and although a small disadvantage: the naming is quite different now between inds_land and indices_allocation_area. Suggest to keep the original naming.

JoostBuitink

LGTM!

verseve · 2024-11-04T09:37:21Z

As briefly disscussed with @verseve, I am in favor of splitting the parameters and the states (i.e. the unknowns you are solving for) in your data structure, but let's leave that as a follow up. The main reason for splitting those is that it is more in alignment with the SciML standard.

Yes, note that this PR made a start with this by using the fields parameters and variables for the different sub-models of LandHydrologySBM (refactoring of vertical SBM concept).

vers-w and others added 30 commits July 1, 2024 10:29

WIP: refactor SBM interception

91b4019

WIP: add AtmosphericForcing to store meteo input

627a9e1

WIP: remove unused type parameter A from SBM

e1a1491

WIP: add RutterInterceptionModel

a6f703c

WIP: refactor SBM snow

4539ddb

Stop using local JULIAUP_DEPOT_PATH (#438) (#440)

5ab02bf

* Stop using local JULIAUP_DEPOT_PATH * Avoid error when override is already set Co-authored-by: Martijn Visser <[email protected]>

WIP: refactor SBM glacier

86371da

WIP: introduce types NoSnowModel and NoGlacierModel

4c109ab

WIP: model in update functions and add wrapper methods

8314006

In `update` functions use `model` for interception, snow and glacier models. Add wrapper methods for snow runoff and glacier melt.

WIP: start refactor soil model of SBM (initialization)

3b904b4

Update of SBM soil model (until recharge)

6f0c958

Delete soil folder

cf8c3be

Add SBM functions to seperate file (bucket_process.jl)

9e84d9a

Also removed the vertical_process.jl file.

Add water demand functionality

25cad4e

Merge branch 'master' into v1

9819e21

Merge branch 'v1' into refactor/sbm_concept

c4837e1

Mostly water demand and allocation functionality. All tests pass.

Rename vegetation_parameters to vegetation_parameter_set

dcada29

Fix test Wflow ZMQ Server

cacbe25

Refactor update of LandHydrologySBM

0d3644e

Add water demand and allocation, and snow transport computations.

Further refactor of update LandHydrologySBM

fc754dd

Add separate functions to update the boundary conditions of the snow and `SBM` models.

Include atmospheric_forcing as a separate argument

9a188b1

In update of models and boundaries of `LandHydrologySBM`

Remove land_parameter_set

2ae54ac

It only contains two parameters that are not shared that much across different models of `LandHydrologySBM`.

Rename outer constructors and couple of structs

33f3f38

Use the struct name for outer constructor functions.

Add parameter soil_fraction to SbmSoilParameters

36d6dd4

Move function for soil_fraction to soil.jl

37249ec

This was linked to issues Oct 18, 2024

Remove vertical concepts HBV and FLEXTopo #431

Closed

Initialization of LateralSSF variables ssf and ssfmax with ksat_profile exponential_constant #473

Closed

Enforce style guide for Julia code #435

Closed

Update changelog

455f1ef

verseve requested a review from JoostBuitink October 21, 2024 08:04

verseve marked this pull request as ready for review October 21, 2024 08:04

verseve self-assigned this Oct 21, 2024

verseve added the v1.0 Activities related to v1.0 label Oct 21, 2024

vers-w added 2 commits October 23, 2024 09:23

Merge branch 'master' into refactor/sbm_concept

429dfd8

Format Wflow.jl

764ab16

SouthEndMusic reviewed Oct 28, 2024

View reviewed changes

vers-w and others added 5 commits October 28, 2024 09:39

Suggestions from code review for water_demand

20c4921

Co-authored-by: Bart de Koning <[email protected]>

Address remaining review comments water_demand

91604d6

Remaining suggestions from code review

cc30210

Co-authored-by: Bart de Koning <[email protected]>

Address review comments

0e06992

Simplify external constructors.

Address review comment

6612822

Rewrite without explicit loop.

JoostBuitink requested changes Oct 28, 2024

View reviewed changes

JoostBuitink reviewed Oct 28, 2024

View reviewed changes

test/run_sbm_piave.jl Show resolved Hide resolved

vers-w and others added 6 commits October 29, 2024 09:36

Address review comment and bug fix

cdf73b6

-Use consistently `!` for functions that are mutating, and return always `nothing` for these functions. - Fix missing `canonicalize` from `Dates`

Fix typo

b1925ee

Co-authored-by: JoostBuitink <[email protected]>

Address remaining comments 2nd reviewer

fca7d52

Merge branch 'refactor/sbm_concept' of https://github.com/Deltares/Wf…

e0427b6

…low.jl into refactor/sbm_concept

Fix wflow server tests

4bc2697

Revert back to using isnothing

8a999fb

As of Julia 1.7 there is no difference and `isnothing` is also used in other code sections.

SouthEndMusic reviewed Nov 4, 2024

View reviewed changes

JoostBuitink approved these changes Nov 4, 2024

View reviewed changes

verseve merged commit c9e5b87 into master Nov 4, 2024
10 checks passed

verseve deleted the refactor/sbm_concept branch November 4, 2024 12:17

vers-w mentioned this pull request Feb 27, 2025

Snow glacier melt #444

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `SBM` concept #487

Refactor `SBM` concept #487

verseve commented Oct 17, 2024 •

edited

Loading

SouthEndMusic left a comment

SouthEndMusic Oct 26, 2024

verseve Oct 28, 2024

SouthEndMusic Oct 26, 2024

verseve Oct 28, 2024

JoostBuitink left a comment

JoostBuitink Oct 28, 2024 •

edited

Loading

verseve Oct 29, 2024

JoostBuitink Oct 28, 2024

verseve Oct 29, 2024

JoostBuitink Oct 28, 2024

verseve Oct 29, 2024

JoostBuitink Oct 28, 2024

verseve Oct 29, 2024

JoostBuitink Oct 28, 2024

verseve Oct 29, 2024

SouthEndMusic left a comment •

edited

Loading

SouthEndMusic Nov 4, 2024

verseve Nov 4, 2024

SouthEndMusic Nov 4, 2024

verseve Nov 4, 2024

JoostBuitink left a comment

verseve commented Nov 4, 2024

Refactor SBM concept #487

Refactor SBM concept #487

Conversation

verseve commented Oct 17, 2024 • edited Loading

Issue addressed

Explanation

Checklist

SouthEndMusic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostBuitink left a comment

Choose a reason for hiding this comment

JoostBuitink Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SouthEndMusic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostBuitink left a comment

Choose a reason for hiding this comment

verseve commented Nov 4, 2024

Refactor `SBM` concept #487

Refactor `SBM` concept #487

verseve commented Oct 17, 2024 •

edited

Loading

JoostBuitink Oct 28, 2024 •

edited

Loading

SouthEndMusic left a comment •

edited

Loading