(WIP) Safer handling of DerivativeOperator.coefficients #244
Conversation
these do not adhere to the documented interface for coeff_func, and also contain a uninitialized memory bug
coeff_func is provided
| """ | ||
| get_coefficient(coefficients::AbstractVector, index::Int) = coeff[i] | ||
|
|
||
| # FIXME: I don't think this case is used anymore |
There was a problem hiding this comment.
This case was covered by a subset of convolution functions, but I don't know what the use-case is.
There was a problem hiding this comment.
It's for a constant coefficient, like D*A so a constant diffusion. If the coefficient is a number, then the whole setup is actually a bitstype so it'll compile to the GPU and distribute much better, and this is a common case so it's worth supporting. I think that one function is really all that's necessary to support it.
| if coeff_func != nothing | ||
| compute_coeffs!(coeff_func, coefficients) | ||
| end | ||
| coefficients = init_coefficients(coeff_func, len) |
|
| get_coefficient(coefficients::Number, index::Int) = coefficients | ||
|
|
||
| # FIXME: Why use "true" here for the value 1? | ||
| get_coefficient(coefficients::Nothing, index::Int) = true |
There was a problem hiding this comment.
true is a hard 1 in Julia. true will convert to any type, so it's the recommended value if you want it to promote in further operations.
| @@ -336,7 +336,7 @@ function LinearAlgebra.Array(A::DerivativeOperator{T,N,true}, len::Int=A.len) wh | |||
| end | |||
|
|
|||
| for i in len-bpc+1:len | |||
| cur_coeff = coeff[i] | |||
| cur_coeff = get_coefficient(coeff, i) | |||
|
These changes are looking great. It's always nice to see a code reduction that keeps functionality, fixes bugs, and is easier to read. |
| @@ -13,7 +13,7 @@ function Base.copyto!(L::AbstractMatrix{T}, A::DerivativeOperator{T}, N::Int) wh | |||
|
|
|||
| coeff = A.coefficients | |||
| get_coeff = if coeff isa AbstractVector | |||
| i -> coeff[i] | |||
| i = get_coefficient(coeff, i) | |||
There was a problem hiding this comment.
Do you reference the variable i before it's defined here? Get an error julia> Array(CenteredDifference{1}(2, 3, 0.01, 100, c_squared)) ERROR: UndefVarError: i not defined Stacktrace: [1] copyto!(::Array{Float64,2}, ::DerivativeOperator{Float64,1,false,Float64,StaticArrays.SArray{Tuple{5},Float64,1,5},StaticArrays.SArray{Tuple{1},StaticArrays.SArray{Tuple{5},Float64,1,5},1,1},Array{Float64,1},Array{Float64,1}}, ::Int64) at /home/simen/Documents/school/masteroppgave/wave_simulation/dev/DiffEqOperators/src/derivative_operators/concretization.jl:16 [2] Array(::DerivativeOperator{Float64,1,false,Float64,StaticArrays.SArray{Tuple{5},Float64,1,5},StaticArrays.SArray{Tuple{1},StaticArrays.SArray{Tuple{5},Float64,1,5},1,1},Array{Float64,1},Array{Float64,1}}, ::Int64) at /home/simen/Documents/school/masteroppgave/wave_simulation/dev/DiffEqOperators/src/derivative_operators/concretization.jl:45 (repeats 2 times) [3] top-level scope at REPL[2]:1 when using a locally merged version of this pull request.
There was a problem hiding this comment.
Changed te = to -> in line 16 as the following lines, which works for me.
This fixes bugs related to referencing undefined memory in the
coefficientsfield ofDerivativeOperator(see #240).I found that the code / docs / tests had inconsistent notions on the type of
coefficients. There were also differences betweenUpwindDifferenceandCenteredDifferencethat didn't have obvious motivation, and weren't mentioned in the docs. I hope we can agree on something consistent and include it in this PR.Currently, I'm hitting a breaking issue dispatching on
convolve_BC_left!(breaks the tests). For a MWE, changeexamples/heat_equation.jlto useUpwindDifferenceinstead ofCenteredDifference.Based on the stacktrace, I can't figure out why it won't choose the method at
convolutions.jl:106:function convolve_BC_left!(x_temp::AbstractVector{T}, x::AbstractVector{T}, A::DerivativeOperator{T,N,true}; overwrite = true) where {T<:Real, N}.I check the type of each arg in the debug log below, then trigger the dispatch error: