search

MagneticResonanceImaging / MRIReco.jl

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

add ecalib for 3D acq and test #117

Closed aTrotier closed 1 year ago

aTrotier commented 1 year ago

Maybe we should wait until the other PR is resolved #115 ? Is it possible to directly dispatch when acq is 2D or 3D ?

codecov-commenter commented 1 year ago

Codecov Report

Base: 66.68% // Head: 66.78% // Increases project coverage by +0.09% :tada:

Coverage data is based on head (5604ad5) compared to base (0f9c32a). Patch coverage: 71.87% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #117 +/- ## ========================================== + Coverage 66.68% 66.78% +0.09% ========================================== Files 37 37 Lines 1696 1728 +32 ========================================== + Hits 1131 1154 +23 - Misses 565 574 +9 ``` | [Impacted Files](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/117?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/117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging#diff-c3JjL1Rvb2xzL0NvaWxTZW5zaXRpdml0eS5qbA==) | `73.17% <71.87%> (-0.24%)` | :arrow_down: | 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

yes, we should extend it to ND but let's do it after https://github.com/MagneticResonanceImaging/MRIReco.jl/pull/115.

tknopp commented 1 year ago

I took the tests from this PR and extended the espirit high level function to handle 3D data as well. Thank you for preparation, this was very helpful! The code is here: https://github.com/MagneticResonanceImaging/MRIReco.jl/commit/dee443c9db8d7b262bfeb4af9b1763a91160f7c2

In the long run, I want AcquisitionData to be parametric in D. Now I needed to dispatch on the calibsize dimension which only works if it is explicitly passed.

aTrotier commented 1 year ago

Great! In the long term do you think we should also be able to dispatch between Cartesian and non Cartesian ? We have a lot of if case or @error from this difference.

tknopp commented 1 year ago

yes probably. We once had something in that direction (a dedicated type hierarchy for trajectories) but that was not really working well. I will have a look at that when looking at making AcquisitionData parametric.