MagneticResonanceImaging / MRIReco.jl

Julia Package for MRI Reconstruction
https://magneticresonanceimaging.github.io/MRIReco.jl/latest/
Other
85 stars 22 forks source link

Type Stability of Normal Operator #51

Open JakobAsslaender opened 2 years ago

JakobAsslaender commented 2 years ago

I am wondering if we should also re-write the way we construct the normal operator depending on whether we want to use a Toeplitz Operator or not. Unfortunately, the function

https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/355a5c89b6e7df9b0e6cb9e72336663d468687b0/src/Operators/NFFTOp.jl#L105-L111

is not type stable:

julia> @code_warntype SparsityOperators.normalOperator(F_nfft)
Variables
  #self#::Core.Const(SparsityOperators.normalOperator)
  S::NFFTOp{ComplexF64}

Body::Union{NormalOp{NFFTOp{ComplexF64}, UniformScaling{Bool}}, MRIReco.Toeplitz_NormalOp{_A, _B, UniformScaling{Bool}} where {_A, _B}}
1 ─ %1 = (#self#)(S, MRIReco.I)::Union{NormalOp{NFFTOp{ComplexF64}, UniformScaling{Bool}}, MRIReco.Toeplitz_NormalOp{_A, _B, UniformScaling{Bool}} where {_A, _B}}
└──      return %1

Any ideas on how we could do this?

tknopp commented 2 years ago

Type instabilities are not always evil and actually it is a feature of Julia that you can mix dynamic and static code. In this particular case I don't think that it will be an issue. The question is what happens with the return type of normalOperator. If you pass it to a solver the code within the solver will be fast because there was a function barrier.

Basically a reconstruction chain on a high-level will always be dynamic, i.e. the operator types being constructed will depend on the input parameters (e.g. non equidistant sampling points -> NFFT, otherwise -> FFT). One the operators a constructed all low-level code should be fast.

The danger is if one has type-unstable structs and passes them from high to low level.