Refactor types and model initialization

[SouthEndMusic]

Explanation

When starting to look into water balance calculations for Wflow, I came across some aspects of the code that I think can be improved:

• Cleaning up dispatching on model type via AbstractModel
• Explicitly write out struct(s) for Model.Network instead of using a nested NamedTuple
• Add inline documentation to network.jl
• Explicitly write out struct(s) for Model.Lateral
• Also structs needed for Model.vertical?
• Clean up initialize_sbm_gwf_model
• Clean up initialize_sbm_model
• Clean up initialize_sediment_model
• Update documentation where required

[vers-w]

Nice, some of these improvements (e.g. write out structs for Network and Lateral) are part of remaining refactoring work for v1.0.!

About the naming I think it would be good to replace the terms lateral and vertical with routing and land_surface (or a similar term), respectively. Both terms are quite common for land surface models (LSM), hydrological models, sediment models etc., used in scientific literature, and we also already use the term routing quite a lot in the code and file/dir names.

What are your thoughts on this @SouthEndMusic and @JoostBuitink ?

[SouthEndMusic]

I don't have opinions yet about hydrological jargon 😋

[SouthEndMusic]

@vers-w I'm done refactoring for this PR :) The structs I introduced also need some inline documentation, but I'm not the right person to do that. I leave it up to you whether you want to do this within this PR or in a follow up.

I see there is a lot of duplicate code in the model initialization code, but I don't understand it well enough at the moment to split it op into sensible sub functions.

[JoostBuitink]

Nice, some of these improvements (e.g. write out structs for Network and Lateral) are part of remaining refactoring work for v1.0.!

About the naming I think it would be good to replace the terms lateral and vertical with routing and land_surface (or a similar term), respectively. Both terms are quite common for land surface models (LSM), hydrological models, sediment models etc., used in scientific literature, and we also already use the term routing quite a lot in the code and file/dir names.

What are your thoughts on this @SouthEndMusic and @JoostBuitink ?

I agree that replacing the terms lateral and vertical is a good idea. I'll think a bit more about it, but agree that routing and land_surface are good options!

[vers-w]

I agree that replacing the terms lateral and vertical is a good idea. I'll think a bit more about it, but agree that routing and land_surface are good options!

Yes, I think replacing the term lateral by routing is a good idea. Not completely sure about the use of land_surface. I think the term land is better (that then for example refers to an AbstractLandModel). With the term land_surface it seems (to me) more about surface processes (e.g. LSMs) and for the CSDMS standard names surface is used to point to processes near the land surface (e.g. infiltration) or on the land (e.g. land_surface_water (water above the land surface)).

[JoostBuitink]

I agree that replacing the terms lateral and vertical is a good idea. I'll think a bit more about it, but agree that routing and land_surface are good options!

Yes, I think replacing the term lateral by routing is a good idea. Not completely sure about the use of land_surface. I think the term land is better (that then for example refers to an AbstractLandModel). With the term land_surface it seems (to me) more about surface processes (e.g. LSMs) and for the CSDMS standard names surface is used to point to processes near the land surface (e.g. infiltration) or on the land (e.g. land_surface_water (water above the land surface)).

I also agree that land_surface doesn't fully "feel" right, hence why I wanted to think a bit more about it. Currently we call the SBM concept land_hydrology (see line below), so perhaps land would indeed be a better name. If I get any other idea's, I'll post them here!

Wflow.jl/src/sbm_model.jl

Line 88 in fbc2383

land_hydrology = LandHydrologySBM(dataset, config, river_fraction, indices)

I fully agree that routing is a logical option!

[vers-w]

@vers-w I'm done refactoring for this PR :) The structs I introduced also need some inline documentation, but I'm not the right person to do that. I leave it up to you whether you want to do this within this PR or in a follow up.

Yes, I can do that in this PR, that's fine. When we have decided what terms to use for vertical and lateral, I can change these and add some inline documentation in one go.

I see there is a lot of duplicate code in the model initialization code, but I don't understand it well enough at the moment to split it op into sensible sub functions.

Yes, definitely true and also something we would like to improve for v1.0. Did not look into detail at this yet. Probably good to check if we can use structs here that we can set partly (I see that you added Accessors as a dependency, so that should be easier/possible now?) and also pass around in the different functions (mostly the routing options have a lot of args). I would say this is for another PR?

[SouthEndMusic]

Yes, in this PR I also make quite heavy use of partly setting structs. With @kwdef I add trivial default values which are set if no data is supplied. Accessors can then be used to set a new value (it makes a new immutable struct from the old one with the new value for a particular field).

[vers-w]

The refactoring LGTM. I did add the inline documentation to the introduced network structs, renamed vertical to land and lateral to routing of the Model struct, and made some small improvements.

Some general comments:

• The introduced network structs have some common fields. I think it could be useful to restructure these a bit, this could be part of another PR. For example, for reservoirs and lakes I did introduce a NetworkWaterBody struct.
• You already mentioned that the model initialization functions have quite some duplicate code. As suggested we could also do this in another PR, including the initialization of the network structs (e.g. move the initialization to network.jl).
• The documentation is not updated (except for the Model struct). We could do this in another PR, probably when the work on using standard names is finished.

[vers-w]

I also agree that land_surface doesn't fully "feel" right, hence why I wanted to think a bit more about it. Currently we call the SBM concept land_hydrology (see line below), so perhaps land would indeed be a better name. If I get any other idea's, I'll post them here!

Hi @JoostBuitink , I did rename vertical to land and included the abstract type AbstractLandModel. If you have any other ideas, let us know!