MagneticResonanceImaging / MRIReco.jl

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

Name of function "data" #38

Closed JakobAsslaender closed 1 year ago

JakobAsslaender commented 2 years ago

Hi, I am wondering if it might be worth renaming the function "data". I know, renaming function breaks code etc., but so does a a function name that people very commonly use for, well, data ...

Thinking a little bit broader: I did not realize that this MRIReco.jl has this functionality, otherwise I would not have re-invented the wheel with BartIO.jl -- but now I wonder if we should get rid of one of the two in order not so we don't have to maintain two versions. I would immediately throw out my package, if it were purely for the readfcfl function I wrote, but now that @aTrotier has written a writecfl function and and a pycall interface to directly interact with BART, it might be worth keeping...

Let me know what you think and if you want to merge the code somehow!

aTrotier commented 2 years ago

Agreed with @JakobAsslaender. I think it is better to keep the BartIO separated. It make sense to keep the pycall wrapper separated with the read/write function.

I see in the function that you vectorize all the data and not returning the dims. Is there a reason ?

Maybe we either read the data in the k-space form then call another function to construct an acquisitionData from the dimensions knowing what the dims means 1- read / 2-phase / 3-slice / 4- Channel / 5- maps / 6-TE etc

tknopp commented 2 years ago

thoughts:

aTrotier commented 2 years ago
  • I have no detailed experience with the data format and one question would be if it is feasible to return a RawAcquisitionData or an AcquisitionData object with it (i.e. is it self-contained). If this is feasible this would be perfect.

The main issue is that the BART format does not store any FOV, orientation etc information. It is only a matrix of k-space (or rawdata with an associated trajectory files). The AcquisitionData will be mainly empty but except for the kdata. We can extract from it (looking for zeros) the subsample indices and encoding size but for the rest it has to be done by hand

  • Having some (small!) test data would help us adding a test. Would be much appreciated if you could provide one.

I have one in BartIO I can create more of them using the phantom function of BART : https://github.com/aTrotier/BartIO.jl/tree/master/test/data

# Dimensions
64 64 1 2 1 1 1 1 1 1 1 1 1 1 1 1 
# Command
phantom -x 64 -k -s 2 data 
# Files
 >data
# Creator
BART v0.7.00-dirty
  • I would put this code in MRIReco.jl. We already of ISMRMRD and BrukerFile so there is no reason to keep that separated. In particular if we can obtain AcquisitionData the integration with MRIReco is highly beneficial. I am open to split MRIReco up into several smaller subpackages if it gets too large. But this is a long term thought, we can do that step by step.

The IO possibility can be kept inside but internally calling the write/read function from BartIO ?

  • On the other hand, the BART Pycall interaction should at this point not be part of MRIReco.jl. I don't want python dependencies if not needed. Furthermore, BART or Gadgetron integration are things that should sit ontop of MRIReco.

Agreed, you only need to have access to the read/write function.

tknopp commented 2 years ago

The IO possibility can be kept inside but internally calling the write/read function from BartIO?

I would suggest moving all that code into MRIReco (despite the Python stuff). My implementation can be removed.

tknopp commented 1 year ago

The original issue with data has been resolved, I am thus closing this issue.