Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor sediment model #463

Draft
wants to merge 9 commits into
base: v1
Choose a base branch
from
Draft

Refactor sediment model #463

wants to merge 9 commits into from

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Sep 9, 2024

Issue addressed

Fixes #382

Explanation

For now this is a tentative to restructure the vertical part (ie soil erosion) of the sediment model.

General

The main features of the restructuration are:

  • I think I would like to separate the current sediment.jl which contain everything either in two or three files: soil erosion, sediment transport (land and river) -> a new erosion.jl file has been created for the soil erosion part and the SoilLoss struct and a new sediment_flux.jl for OverlandFlowSediment and RiverSediment
  • hydrometeo_forcing now contains all forcing for sediment whether they will be used by the land or river part. Not sure if this will be the final solution.
  • new RiverGeometry and LandGeometry struct that contains cell properties such as width, slope, length, area etc to avoid defining and passing these parameters that can be common to many structs.

TODO left:

  • BMI and @grid_loc
  • Rename the initialise functions of the main structs (SoilLoss, LandSediment, RiverSediment)
  • Update the test staticmaps

Vertical: SoilLoss

The main features of the restructuration are:

  • Three main structs have been created AbstractRainfallErosionModel (with three models NoRainfallErosionModel, RainfallErosionEurosemModel, RainfallErosionAnswersModel), AbstractOverlandFlowModel (with two models NoOverlandFlowErosionModel, OverlandFlowAnswersModel) and SoilErosionModel which is in charge of summing erosion from the different sources (for now rainfall and overland flow but in the future we could add wind, mass wasting from landslide etc.) and differentiating for the different particle classes. Each struct is defined in its own script but the equations are in a common file (erosion_process.jl)
  • very few parameters are common to each concept so for now I did not create a separate erosion_parameters (similar to vegetation_parameters in sbm)
  • area is the only separate parameter of the main SoilLoss struct, timestep is passed directly to the update functions.
  • The computation of the transport capacity has been moved to the land transport and is no longer part of the soil erosion (vertical concept)
  • The calculations of the soil fractions of clay/silt/sand/small+large aggregates has been moved to HydroMT

TODO left:

  • Computation of interception and canopygapfraction should re-use the interception model from the sbm refactor

Lateral.land: OverlandFlowSediment

The main features of the restructuration are:

  • The main struct is composed of a transport_capacity model that can link to three different concepts with/without particle differentiation (TransportCapacityGoversModel, TransportCapacityYalinModel, TransportCapacityYalinDifferentiationModel); a sediment_flux model for the overland flow transport with two equations for with/without particle differentiation and a to_river to summarize the sediment reaching the river (with/without particle differentiation).
  • The to_river model is only masking overland flow deposition for the river cells. It may not be needed but I started to like the concept that for vertical, lateral, you can call the different output in a generic way ie vertical.soil_erosion.clay, lateral.land.transport_capacity.clay, lateral.land.to_river.clay (actually with a an extra variables in between vertical.soil_erosion.variables.clay). I think this structure is also more straightforward than arranging around particles to make it easy to use different concepts or for dependencies on particles for certain processes (ie not an organization like vertical.clay.variables.soil_erosion or lateral.land.clay.variables.transport_capacity).

TODO left:

  • vertical and lateral.land share two common forcing (q_land and waterlevel_land). I am not sure if there would be a smart way to avoid double definition / double reading in the sediment_config.toml of these variables part from passing the vertical.hydrometeo_forcing struct as an input to lateral.land.update).

Lateral.river: River Sediment

I thought I would be able to split more but because we need the upstream to compute the downstream, I ended up with less structures than planned and still quite a large structure for the mass balances (that still include then erosion and deposition). In the end this means the construction looks similar to lateral.land so this may not be for the worst. I also could not easily separate the reservoirs into an extra struct so I left it as is in master.
The main features of the restructuration are:

  • The main struct is composed of a transport_capacity model that can link to five different concepts (TransportCapacityBagnoldModel, TransportCapacityEngelundModel, TransportCapacityYangModel, TransportCapacityMolinasModel, TransportCapacityKodatieModel); a potential_river_erosion model to get the maximum allowed direct bed and bank erosion (for example in the future to use delwaq concept this could be turned into none allowed); a sediment_flux model for the river flow transport (mass balance from upstream to downstream including deposition and erosion); and a concentrations to convert loads to concentrations and get the suspended vs bed part.

TODO left:

  • a lot of particle parameters are now duplicated (density, mean diameter etc) and could be moved into separate Particle struct

Testing of this PR

  • Because some parameter calculations have moved to hydromt, a new version of the staticmaps needs to be published for the test to pass
  • The test results differ a little in v1 because pathfrac is no longer used with the answers concept (pathfrac is actually part of the usle_c parameter so it was counting double)

Checklist

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

Additional Notes (optional)

Add any additional notes or information that may be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant