avoid assigning storage for nothing situation
#62
Conversation
src/Denudation/Abstract.jl
Outdated
| @@ -24,7 +24,13 @@ function denudation(input) | |||
| end | |||
|
|
|||
| if water_depth[idx] >= 0 | |||
| (denudation_mass[idx]) = denudation(input.box, input.denudation, water_depth[idx], slope[idx], input.facies[f]) | |||
|
|
|||
| if denudation(input.box, input.denudation, water_depth[idx], slope[idx], input.facies[f]) !== nothing | |||
There was a problem hiding this comment.
You're calling this function twice here. In many cases the compiler will optimize this away, but since it is quite a long call, it is more hygienic to write it differently.
src/Denudation/Abstract.jl
Outdated
| @@ -14,7 +14,7 @@ abstract type DenudationType end | |||
| FIXME Computes the denudation for a single time-step, given denudation parameters `param` and a simulation state `state`. `param` should have a `DenudationType` type and `state` should contain the `height` property and `sealevel`. | |||
| """ | |||
| function denudation(input) | |||
| denudation_mass::Array{typeof(1.0u"m/kyr"),2} = Array{typeof(1.0u"m/kyr")}(undef, input.box.grid_size...) | |||
| denudation_mass::Union{Array{typeof(1.0u"m/kyr"),2}, Nothing} = Array{typeof(1.0u"m/kyr")}(undef, input.box.grid_size...) | |||
There was a problem hiding this comment.
We're trying to prevent unneeded memory allocations. Here, what you're saying is: I have this variable denudation_mass which is either an array or nothing, and then you assign a big empty array to it. That doesn't make much sense.
There was a problem hiding this comment.
right I can initialise with nothing...
There was a problem hiding this comment.
The two components of the model have different approaches: the denudation part works on a per-pixel basis, while the redistribution part works integral on the entire grid. You chose to do it that way because the redistribution part required this.
Now, for the best flexibility, we need to have the same dispatch mechanism on both denudation and redistribution. So the denudation function should also work on the entire grid. That way the nothing case falls into place much more naturally. Also, this allows future extensions where the denudation function can compute something more complex.
Since we need a dispatch on the type of Denudation, we'll need to move the for loop in line 20 of Abstract.jl, down one level:
denudation(input) = denudation(input, input.denudation)
denudation(input, denudation::T) where T <: DenudationType
<<current implementation of denudation(input)>>
end
denudation(input, denudation::NoDenudation)
return state -> nothing
endOr something along that line of thinking.
I'll make a new issue out of it. The problem to change to matrix-wise is how to deal with facies. |
|
should be in issues/65 |
|
Please don't. A nice and consistent solution for this issue is only possible if you do it in the way I described. No need to create new issues. If you have questions about this please get in touch. |
src/Denudation/Abstract.jl
Outdated
| (denudation_mass[idx]) = denudation(input.box, input.denudation, water_depth[idx], slope[idx], input.facies[f]) | ||
| denu_result = denudation(input.box, input.denudation, water_depth[idx], slope[idx], input.facies[f]) | ||
| if denu_result !== nothing | ||
| denudation_mass = Array{typeof(denu_result)}(undef, size(state.ca)...) |
There was a problem hiding this comment.
Now you're creating a new Array filled with undefined values at every iteration. Please follow my previously sketched solution.
There was a problem hiding this comment.
I tried to now make the denudation function running on all grids by down the for loop on level (i.e., encapsulate for loop within individual denudation function). By doing so, I can allocate storage within each denudation function and no need to use Union{Array{typeof(1.0u"m/kyr"),2}, Nothing} and avoiding generating redundant arrays. I didn't do the broadcasting way because I haven't find an efficient way to deal with the cell-specific facies. However, the denudate function in Abstract.jl was now extremely shortened and I am not sure whether it is still should be there. I kept it now because try to keep consistency with redistribution. The codes itself is runnable, but I have to upgrade tests yet because the tests were still designed to work on pixel-wise denudation function. If this way works, I would then upgrade the test.
No description provided.