search

MagneticResonanceImaging / MRIReco.jl

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

raw.encodedFOV in mm and acq.fov in meter #137

Closed aTrotier closed 1 year ago

aTrotier commented 1 year ago

Issue

raw.params["encodedFOV"] is currently in meter when I read a BrukerFile and it is in mm for read_dir... which cause some issue later when I want to correct the offset. According to this, I think encodedFOV should be in mm ? https://github.com/ismrmrd/ismrmrd/blob/1b260407a69195a6666844e9706ff20ba965ef29/schema/ismrmrd_example.xml#L26

But for AcquisitionData : 

* `fov::Vector{Float64}`                    - field of view in m

This PR corrects :

discusion

Should we put fov in mm for AcquisitionData ?

What functions are using acq.fov ?

tknopp commented 1 year ago

The BrukerFile change looks correct, thanks.

Regarding AcquisitionData I think it is already in mm, see https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/c48dfc53051ff24132b8be91f0c5e8174fa80315/src/Tools/ImageData.jl#L14

Hence, i think you need to remove the division by 1000.

aTrotier commented 1 year ago

Here it says it is in meter... https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/c48dfc53051ff24132b8be91f0c5e8174fa80315/MRIBase/src/Datatypes/AcqData.jl#L18

tknopp commented 1 year ago

Here it says it is in meter...

Ah ok then the documentation is wrong. My take away from https://github.com/MagneticResonanceImaging/MRIReco.jl/issues/126 was that we handle everything in mm.

Can you change the documentation in your PR (+ revert the factor 1000)?

codecov-commenter commented 1 year ago

Codecov Report

Base: 63.05% // Head: 63.05% // No change to project coverage :thumbsup:

Coverage data is based on head (03986e3) compared to base (f03a934). Patch has no changes to coverable lines.

:exclamation: Current head 03986e3 differs from pull request most recent head 2eae075. Consider uploading reports for the commit 2eae075 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #137 +/- ## ======================================= Coverage 63.05% 63.05% ======================================= Files 8 8 Lines 249 249 ======================================= Hits 157 157 Misses 92 92 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

aTrotier commented 1 year ago

I will try to acquire a new 3D datasets with offsets that we can use instead of the current one.