search

MagneticResonanceImaging / MRIReco.jl

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

reco of repetitions along dims=6 #105

Closed aTrotier closed 1 year ago

aTrotier commented 1 year ago

in this PR :

Notes

codecov-commenter commented 1 year ago

Codecov Report

Base: 64.72% // Head: 64.69% // Decreases project coverage by -0.02% :warning:

Coverage data is based on head (8e70054) compared to base (f3a3b7b). Patch coverage: 59.25% of modified lines in pull request are covered.

:exclamation: Current head 8e70054 differs from pull request most recent head fee105c. Consider uploading reports for the commit fee105c to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #105 +/- ## ========================================== - Coverage 64.72% 64.69% -0.03% ========================================== Files 37 37 Lines 1661 1671 +10 ========================================== + Hits 1075 1081 +6 - Misses 586 590 +4 ``` | [Impacted Files](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging) | Coverage Δ | | |---|---|---| | [src/Reconstruction/IterativeReconstruction.jl](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/105/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging#diff-c3JjL1JlY29uc3RydWN0aW9uL0l0ZXJhdGl2ZVJlY29uc3RydWN0aW9uLmps) | `46.78% <50.00%> (+0.25%)` | :arrow_up: | | [src/Reconstruction/DirectReconstruction.jl](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/105/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging#diff-c3JjL1JlY29uc3RydWN0aW9uL0RpcmVjdFJlY29uc3RydWN0aW9uLmps) | `95.83% <100.00%> (+0.37%)` | :arrow_up: | | [src/Tools/ImageData.jl](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/105/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging#diff-c3JjL1Rvb2xzL0ltYWdlRGF0YS5qbA==) | `100.00% <100.00%> (ø)` | | | [src/Tools/Nifti.jl](https://codecov.io/gh/MagneticResonanceImaging/MRIReco.jl/pull/105/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MagneticResonanceImaging#diff-c3JjL1Rvb2xzL05pZnRpLmps) | `100.00% <100.00%> (ø)` | | 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.

migrosser commented 1 year ago

Looks pretty nice :) I think it might be good to rename the method AcqDataFromKspace to something which makes it clear that we are referring to Cartesian data here. Maybe something like AcqDataFromCartesianKspace ?

Should we change order to x,y,z*slice,channels,echoes,repetitions ?

This only refers to the dimensionality of the reconstructed images, right? In that case I am in favor of changing the order.

it would be great to be able to set different trajectory for each dimension (not only the echo ones)

For the AcquisitionData and the reconstruction itself it should not be too hard to do this. Maybe I have some time to come up with a draft for this tomorrow. A potential pitfall is that we need to make sure not to break the conversion from RawAcquisitionData to AcquisitionData.

aTrotier commented 1 year ago

Looks pretty nice :) I think it might be good to rename the method AcqDataFromKspace to something which makes it clear that we are referring to Cartesian data here. Maybe something like AcqDataFromCartesianKspace ?

For now, the conversion is only for cartesian k-space. But later I might enhanced it to extract non-cartesian "kdata" in a format close to the one used by Bart. @tknopp @JakobAsslaender I am mostly using thus kind of function to create kdata for BartIO. Should we keep them in MRIReco ?

This only refers to the dimensionality of the reconstructed images, right? In that case I am in favor of changing the order. I think so. acq.kdata are stored with dimension (echoes,slice,NR) but it is not directly one array and we don't manipulate them directly.

For the AcquisitionData and the reconstruction itself it should not be too hard to do this. Maybe I have some time to come up with a draft for this tomorrow. A potential pitfall is that we need to make sure not to break the conversion from RawAcquisitionData to AcquisitionData.

Yes it is a major modifications in MRIReco but it make sense to be able to store trajectory for each dimensions particularly for retrospective binning. I already have an example that requires at least 2 dimensions to store cardiac and respiratory non-cartesian kdata (I can use the echo for one and repetition for the other)

migrosser commented 1 year ago

For now, the conversion is only for cartesian k-space. But later I might enhanced it to extract non-cartesian "kdata" in a format close to the one used by Bart.

That is a good point when we are talking about extracting k-space data from the AcquisitionData. For AcqDataFromKspace, I do not really see how the method could be generalized to non-Cartesian data without passing the sampling trajectory with it. However, as soon as one has to do that, the method ends up doing the same thing as the constructor for AcquisitionData.

aTrotier commented 1 year ago

For non-cartesian reconstruction Bart expects a kdata files with dimensions : 1, readout, number of projection,channel, ... associated with a trajectory files : 3 (kx,ky,kz), readout,projection,...

migrosser commented 1 year ago

What if we just overload the constructor and thus call the method AcquisitionData as well? I think that way the naming would be the most coherent. If I am not mistaken, the dispatch should work just fine. If you pass the kdata along with a trajectory our current standard constructor would be called. If you only pass k-data, the data would be assumed to be Cartesian and your current implementation of AcqDataFromKspace would be called. For the non-Cartesian data we could then implement another constructur, which takes the kdata and another array containing the k-space nodes as input.

migrosser commented 1 year ago

Another really small thing: currently AcqDataFromKspace seems to assume a 3d-dataset. Maybe we should put a note into the docstring stating that this method does not allow to process data which contains multiple 2d-encoded slices...just to avoid confusion.

aTrotier commented 1 year ago

Seems great. Interface will be coherent and easy to remember :)

for cartesian k-space something like :

function AcquisitionData(kdata::Matrix{Complex{T}}
                        ; seqInfo=Dict{Symbol,Any}()
                        , fov=Float64[0,0,0]
                        , kargs...) where T <: AbstractFloat

return AcqDataFromKspace(kdata)
end
migrosser commented 1 year ago

I just did the changes. However, I directly renamed AcqDataFromKspace instead of wrapping it into another function, which does basically the same thing. Do you have any use-case that would require us to keep both versions (i.e. AcquisitionData(kdata; kargs...) and AcqDataFromKspace?

Concerning the option of setting different trajectories also for the repetitions, I just had a look at it. Its certainly possible to do that. However, I would have adapt quite a few number of things. Moreover, I think it would be nice to use the opportunity to make AcquisitionData a bit smarter in the sense that we no longer need to allocate separate trajectories for each contrast/repetition if the same one is to be used for all of them. Taking this into account, I think it would be best to merge this PR and tackle the trajectory issue in a next step.

What do you think about this? If the current state of the PR is good for you, I will merge it tomorrow.

aTrotier commented 1 year ago

I don't see a use-case for a dedicated function.

The last commit use a 2D trajectory when 3rd dimension = 1. Now the reconstruction parameters is more coherent params[:reconSize] = (N,N,1) -> params[:reconSize] = (N,N)

If you are ok with the last commit, you can merge the PR.