MagneticResonanceImaging / MRIReco.jl

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

Adjustment of subsampleIndices when changing encoding size #112

Closed alexjaffray closed 1 year ago

alexjaffray commented 1 year ago

I was having trouble using espirit to estimate sensitivity maps of arbitrary resolution from low resolution data.

I noticed that the subsampleIndices were not being updated when the encoding size was being changed, and the acqData.encodingSize was not being correspondingly changed as well. Espirit depends on the subsamplingIndices being correctly set to get samples from the low-frequency k-space data, and this wasn't being handled. Now, one can estimate arbitrarily sized sensitivity maps from low-res acqData if you first change the encodingSize.

This PR seems to fix the issue and changeEncodingSize2D seems to work as expected, please review when you can. Also, it might be useful to make changeEncodingSize2D! accessible outside of the package scope. I didn't add that in my changes though so it's up for discussion.

codecov-commenter commented 1 year ago

Codecov Report

Base: 65.46% // Head: 65.76% // Increases project coverage by +0.30% :tada:

Coverage data is based on head (356cc11) compared to base (ab0dd9d). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #112 +/- ## ========================================== + Coverage 65.46% 65.76% +0.30% ========================================== Files 37 37 Lines 1694 1744 +50 ========================================== + Hits 1109 1147 +38 - Misses 585 597 +12 ``` | [Impacted Files](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging) | Coverage Δ | | |---|---|---| | [src/Tools/CoilSensitivity.jl](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/112/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging#diff-c3JjL1Rvb2xzL0NvaWxTZW5zaXRpdml0eS5qbA==) | `64.70% <0.00%> (-0.80%)` | :arrow_down: | | [src/Tools/ErrorMeasures.jl](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/112/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging#diff-c3JjL1Rvb2xzL0Vycm9yTWVhc3VyZXMuamw=) | `100.00% <0.00%> (+100.00%)` | :arrow_up: | 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.

tknopp commented 1 year ago

I try to understand what is happening here:

alexjaffray commented 1 year ago

@tknopp It does seem that it was originally only designed for a reduction in encoding size, but this was not documented. Zero filling would be the intended outcome if the encoding size is increased, and is essentially what is done in this PR. The findIndices function just finds the shifted subsampleIndices so the original data is correctly centered in k-space, achieving essentially zero-filling.

At the moment the use case for this is mostly Cartesian 2D acquisitions, but I could see a general motivation for having the following tools available for N-D and arbitrary trajectory data:

alexjaffray commented 1 year ago

Updated code with error message in same style as espirit error message

alexjaffray commented 1 year ago

Closing this for now and starting new development on espirit wrapper code to solve the underlying issue of decoupling the sensitivity map size from the acqData.encodingSize