MagneticResonanceImaging / MRIReco.jl

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

voxel_size units in saveImage #126

Closed mrikasper closed 1 year ago

mrikasper commented 1 year ago

Dear all,

I have just tried the saveImage function to export reconstructed images to NIfTI.

cartesianReco = reconstruction(acqDataCartesian,paramsCartesian)
saveImage("test2.nii", cartesianReco)

However, when I read the .nii (e.g, in fsleyes), it looks as if the resolution is a factor of 1000 wrong (s. screenshot).

image

From the README of NIfTI.jl, it seems that voxel_size should be in mm, whereas we convert it into m in saveImage - could this be the issue?

If so, one could just change the line to

voxel_size = ustrip.(uconvert.(u"mm", pixelspacing(im)[1:3]))

...I am happy to create a PR, but wanted to check whether there are any downstream consequences or other things to consider before I start.

Our returned object from MRIReco's reconstruction function seems to have the right dimensions, in particular the FOV range and step width.

cartesianReco.axes

(Axis{:x, StepRangeLen{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Int64}}((0.0:1.1:218.9) mm), 
Axis{:y, StepRangeLen{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Int64}}((0.0:1.089000015:216.71100298500002) mm), 
Axis{:z, StepRangeLen{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Int64}}((0.0:2.0:28.0) mm), 
Axis{:echos, UnitRange{Int64}}(1:2), Axis{:coils, UnitRange{Int64}}(1:1), 
Axis{:repetitions, UnitRange{Int64}}(1:1))

I had a brief look at testIO.jl, but it doesn't reproduce a similar error, and the resolution unit of the output .nii seems correct (1 mm). If indeed there is an error, we might want to think of a better test here?

Thank you for your help, Lars

tknopp commented 1 year ago

is this with the currently released version? Just asking, since we moved the NIfTI code to ImageUtils.jl

tknopp commented 1 year ago

So what I don't get is that the NIfTI that you get in the first case wrong and in the other case correct. It would be great if you could have a look at the axes of the reco object before saving for the testIO.jl case.

But in general, we can change m to mm in: https://github.com/tknopp/ImageUtils.jl/blob/master/src/Nifti.jl#L10

I would argue that NIfTI.jl should make a conversion to mm internally but it does not hurt, that we do this before, if that conversion is not yet present in NIfTI.jl.

aTrotier commented 1 year ago

It was also something I noticed on Bruker datasets. That's why I put a /1000.0 somewhere which cause another issue later : https://github.com/MagneticResonanceImaging/MRIReco.jl/pull/121

Le lun. 24 oct. 2022 Γ  21:13, Tobias Knopp @.***> a Γ©crit :

is this with the currently released version? Just asking, since we moved the NIfTI code to ImageUtils.jl

β€” Reply to this email directly, view it on GitHub https://github.com/MagneticResonanceImaging/MRIReco.jl/issues/126#issuecomment-1289479173, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5P7O4ZQSAMHWDYFBOBJWLWE3NTZANCNFSM6AAAAAARNIQ4YI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

TROTIER AurΓ©lien 31 allΓ©e de la Biotte 33470 Gujan-Mestras 06.09.12.61.61 @.***

tknopp commented 1 year ago

yes, the internal value is now considered to be in mm. So we should change the save routine and then everything should be fine I guess.

mrikasper commented 1 year ago

is this with the currently released version? Just asking, since we moved the NIfTI code to ImageUtils.jl

Oh, sorry, no, it isn't. This was still with v0.5.5. I will update as soon as I get the rest of our use case running :-)

So what I don't get is that the NIfTI that you get in the first case wrong and in the other case correct. It would be great if you could have a look at the axes of the reco object before saving for the testIO.jl case.

I think the step size is ill-defined (0mm) in the test case:

Ireco.axes

(Axis{:x, StepRangeLen{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Int64}}((StepRangeLen(0.0, 0.0, 256)) mm), 
Axis{:y, StepRangeLen{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Int64}}((StepRangeLen(0.0, 0.0, 256)) mm), 
Axis{:z, StepRangeLen{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Base.TwicePrecision{Quantity{Float64, 𝐋, Unitful.FreeUnits{(mm,), 𝐋, nothing}}}, Int64}}((StepRangeLen(0.0, 0.0, 1)) mm), 
Axis{:echos, UnitRange{Int64}}(1:1), 
Axis{:coils, UnitRange{Int64}}(1:1), 
Axis{:repetitions, UnitRange{Int64}}(1:1))

But in general, we can change m to mm in: https://github.com/tknopp/ImageUtils.jl/blob/master/src/Nifti.jl#L10

I would argue that NIfTI.jl should make a conversion to mm internally but it does not hurt, that we do this before, if that conversion is not yet present in NIfTI.jl.

I agree that their interface should handle it, but apparently they don't use explicit Unitful info for the voxel_size at the moment. I think you suggestion to alter the one line of code makes in ImageUtils makes a lot of sense, maybe it's fastest if you implement the change, or I can do it after I update to MRIReco v0.7.

All the best, Lars

tknopp commented 1 year ago

I take care and make a release of ImageUtils.

tknopp commented 1 year ago

https://github.com/JuliaRegistries/General/pull/70968

mrikasper commented 1 year ago

Great, thank you so much, Tobias, @tknopp !

tknopp commented 1 year ago

I think this can be closed.