Closed aTrotier closed 2 years ago
Merging #67 (0aeda25) into master (b175aa9) will not change coverage. The diff coverage is
63.63%
.:exclamation: Current head 0aeda25 differs from pull request most recent head 56e72a7. Consider uploading reports for the commit 56e72a7 to get more accurate results
@@ Coverage Diff @@
## master #67 +/- ##
=======================================
Coverage 64.75% 64.75%
=======================================
Files 63 63
Lines 3073 3073
=======================================
Hits 1990 1990
Misses 1083 1083
Impacted Files | Coverage Δ | |
---|---|---|
src/Tools/CoilSensitivity.jl | 67.51% <37.50%> (ø) |
|
src/Datatypes/AcqData.jl | 42.44% <38.46%> (ø) |
|
src/Reconstruction/IterativeReconstruction.jl | 47.31% <55.55%> (ø) |
|
src/IO/Brukerfile.jl | 67.12% <80.00%> (ø) |
|
src/Reconstruction/RecoParameters.jl | 82.69% <80.00%> (ø) |
|
src/IO/HDF5Types.jl | 86.77% <83.33%> (ø) |
|
src/Datatypes/RawAcqData.jl | 72.10% <100.00%> (ø) |
|
src/Reconstruction/DirectReconstruction.jl | 94.73% <100.00%> (ø) |
|
src/Tools/Regridding.jl | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update b175aa9...56e72a7. Read the comment docs.
Could you try making the parametrization with T <: AbstractFloat
? It did this in NFFT.jl:
https://github.com/JuliaMath/NFFT.jl/blob/master/src/implementation.jl#L29
and it allows me to have the same floating point type for real and complex numbers. I think that would be the cleaner solution.
Otherwise, this already looks very good!
Do you prefer to have :
Complex{T} where T <: AbstractFloat
or
T <: Complex{<:AbstractFloat}
?
I have implemented the later one and it works
edit Sorry I did not read that part :
and it allows me to have the same floating point type for real and complex numbers. I think that would be the cleaner solution.
I'll change that :)
I made a quick review with smaller suggestions. Only minor stuff but you need to change it everywhere. Once this is done I think this is ready to go.
By the way: Trajectory
will likely also want to be parameterized but lets do that in another PR.
Weirdly I don't really get any enhancement in reconstruction time/ allocations with F32
[ Info: testCSRecoMultCoil(type = ComplexF64)
6.063130 seconds (5.66 M allocations: 1.767 GiB, 7.69% gc time, 75.76% compilation time)
[ Info: testCSRecoMultCoil(type = ComplexF32)
14.136537 seconds (18.22 M allocations: 2.408 GiB, 7.38% gc time, 4.05% compilation time)
[ Info: testCSRecoMultCoil(type = ComplexF64)
1.352299 seconds (263.78 k allocations: 1.454 GiB, 22.91% gc time)
[ Info: testCSRecoMultCoil(type = ComplexF32)
1.331056 seconds (264.04 k allocations: 1.402 GiB, 20.38% gc time)
I don't expect improvements in reconstruction time. This would only happen for proper SIMD code. Regarding the storage one would need to investigate what these 1.4 GB
actually are, and where it is allocated.
Thanks for the contribution!
Code that calls the 2-argument version of SensitivityOp fails after merging this for 32bit AcquisitionData because of the requirement that AcquisitionData and the SensitivityMaps have the same datatype. Should this go in an experimental branch?
Related to #61
I used a lot of
dType = = typeof(acqData.kdata[1,1,1][1])
Is it ok like that or do you prefer to add a acqdataType in the structAcquisitionData
?I had to use
D <: Complex
becauseComplex{AbstractFloat}
is not working maybe it exists another possibility ? Edit : response from Slack :T <: Complex{<:AbstractFloat}
is suppose to work