Refactor routing #496
Refactor routing #496
Conversation
Split into `boundary_conditions`, `variables` and `parameters`.
Split into `boundary_conditions`, `variables` and `parameters`.
Split into `boundary_conditions`, `variables` and `parameters`.
Bug fix: initialization of `allocation` as part of `ShallowWaterRiver`
Waterbody inflow from the land component (overland and subsurface flow).
Removed `dt` field from model components (vertical and lateral). Model timestep `dt` is now only stored as part of `Clock` struct.
Now similar implementation as local inertial method: stable timestep is computed each sub timestep (or a fixed sub timestep is used) as part of a while loop (for each model timestep).
Similar to kinematic wave approximation. Also added a function for the initialization of timestepping for kinematic wave.
Rename variables to more descriptive names. Bug fix: the reservoir (`reservoir_index_f`) and lake (`lake_index_f`) indices in the function `update_water_allocation!` were not correct, these were mapped to their own index and not to the corresponding river index.
Model components river flow, overland flow and groundwater flow.
Not required as a separate function argument.
As if refers to the grid cell area and not the river area.
Consistent use of `edge` (link and edge were both used).
Refer to the kinematic wave method, more explicit and easier to distinquish from other river or overland flow methods.
Refer to the local inertial method, more explicit and easier to distinquish from other river or overland flow methods.
From structs `LakeParameters`, `ReservoirParameters`, `LateralSsfParameters` and `GroundwaterExchangeParameters`.
Change unit (m3 per model timestep) `totaloutflow` (renamed to `outflow_av`) and `inflow` to m3/s . Use functions for setting reservoir or lake variables (`inflow`, `outflow_av` and `actevap`) at the start of each timestep to zero and to compute averages valid for the model timestep for variables `inflow` and `outflow_av`.
Fix computing `dt_min` for kinematic wave: value of NaN could occur when only one cell q > 0.0 (quantile!). Increased default `dt_min` for local inertial routing to 60.0 s.
| k = ncread( | ||
| dataset, | ||
| config, | ||
| "lateral.subsurface.conductivity"; |
There was a problem hiding this comment.
This will probably be tackled by the mapping of parameters, but should this be "lateral.subsurface.parameters.conductivity"? Same for the lines below (specific_yield, gwf_f)
There was a problem hiding this comment.
Yes, this will indeed change when we will move to using standard names.
For consistency it would have been better to change this mapping, but this time I skipped these as it is also not really required for the input parameters (on the other hand, for output it is required). For the SBM refactoring I also changed these mainly to show what is required (for the output) when staying with one-one mapping of parameters and variables.
| Run surface routing (land and river) for a single timestep. Kinematic wave for overland flow | ||
| and kinematic wave or local inertial model for river flow. | ||
| """ | ||
| function surface_routing!(model) |
There was a problem hiding this comment.
Woudn't it be nicer if we specific the type of the model here (same as below for local-inertial overlandflow and riverflow)?
There was a problem hiding this comment.
I guess this is a matter of taste, for me it's fine, dispatching takes care of it anyway.
| @test row.Q_av ≈ 0.01620324716944374f0 | ||
| @test row.Q_av ≈ 0.01619703129434486f0 |
There was a problem hiding this comment.
I don't directly see what causes these changes, do you know why this happened? Same for the other differences in Q in the other test.
There was a problem hiding this comment.
Ah I see in the commit history, it is probably related to the new TimeStepping, correct?
There was a problem hiding this comment.
Yes, it is related to changes in the kinematic wave timestepping, that is now similar to the local inertial routing for consistency. During the loop (for each model timestep) a stable timestep is computed for each sub timestep now, by also indeed making use of the new TimeStepping struct.
Issue addressed
Fixes #510
Explanation
See issue #510 and updated changelog.
Checklist
master