-
Notifications
You must be signed in to change notification settings - Fork 22
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/init sediment model #446
base: v1
Are you sure you want to change the base?
Conversation
A general comment (did not go though the code in detail in yet): what about refactoring of the different structs that are part of the Sediment model? As discussed for the SBM concept we would like to sub-divide long structs in sub-structs, I think this is also applicable for the Sediment concept. Probably good to also ask @hboisgon to review this PR? |
I completely agree and this is also what I intend to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only two small comments.
As suggested, would be good if @hboisgon also reviews this PR.
) | ||
|
||
sediment_model = SedimentModel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the advantage of doing this? Why not use directly SedimentModel()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It facilitates debugging.
Having the construction of an object as a separate statement instead of nesting its construction in the construction of another object adds additional flexibility for placing breakpoints.
For the same reasoning I also refrain from constructing objects during a return statement.
src/sediment_model.jl
Outdated
function init_overland_flow_sediment(river::Vector, number_of_cells::Integer) | ||
river_cell = convert(Vector{Float}, river) | ||
|
||
overland_flow_sediment = OverlandFlowSediment{Float}(; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of Float
you could also use T
here, equal to the SBM
model concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an argument to the function call to generalize the construction of OverlandModelSediment
.
It can now construct OverlandFlowSediment
with type parameters other than Float
as long as it is a subtype of AbstractFloat
.
This PR refactors the initialization of the sediment model by: