Huge Package Split

[tknopp]

I just split off various code chunks out of MRIReco into dedicated packages:

• MRISampling
• MRIOperators
• MRISimulation
• MRICoilSensitivities

The split worked out rather well and I was able to untangle some code, which was not cleanly separated before. I currently register the packages but this will require about 6 days. I recommend to not use master of MRIReco until all packages are registered.

[tknopp]

This is basically how things look like after the split:

In red are some remarks for the future, which are, however, not so relevant for the packages below.

[aTrotier]

Can you share the way to work on dev with all the package ? :)

[tknopp]

Right now just by dev ~/.julia/dev/MRIReco/MRISampling for all packages with an MRI prefix in Pkg mode. Once everything is registered one hopefully does not need to dev all packages but only those, which one is working on.

[tknopp]

I want to elaborate a little bit more on what has / is happening and why I think this is a good thing in the long run. Until now MRIReco was kind of a big leaf in the package tree and when one loaded MRIReco one had a full-featured reconstruction library. In the new world we currently don't have such a "toolbox" package anymore. MRIReco is just supposed to do what is in the name (i.e. image reconstruction) not more. This means people will need to update

using MRIReco

with

using MRIReco
using MRISampling               # only if you plan to subsample data
using MRICoilSensitivities      # only if you plan to estimate coil sensitivities
using MRIFiles                  # only if you read data from files
using MRISimulation             # only if you are simulating MRI data. 

While this seems like a little bit more typing, one will actually benefit from this with shorter load times when not all packages are required. Other than that, the API should be mostly backwards compatible with MRIReco.jl version 0.6. I made one independent API change in MRIBase (AcquisitionData is now parametric in D) but that one is independent of the package split.

One might ask now: Why are all those packages within the MRIReco repository? Would't it be cleaner to have dedicated repositories?

The answer is: Maybe. But in the end it does not matter at all. Packages that are in subfolders are just regular packages. You don't need to be afraid that (for instance) depending on MRICoilsSensitivities would make you dependent on MRIReco. If we want, we can put the packages in individual repositories at some point. But it will not make a difference for package users.

It's also important that MRIBase is a really small package. We right now have

julia> @time_imports using MRIBase
     22.3 ms  AbstractNFFTs
      0.4 ms  Compat
     55.9 ms  ChainRulesCore
      7.2 ms  AbstractFFTs
     14.6 ms  Preferences
      0.3 ms  JLLWrappers
      3.6 ms  FFTW_jll 55.18% compilation time
    180.5 ms  FFTW 10.29% compilation time
      0.8 ms  NFFTTools
      1.8 ms  MRIBase

So in total about 0.3 seconds load time which will vanish if you load FFTW anyway. We could make MRIBase even smaller my creating MRITrajectories but that would be a little bit artificial since we would need to keep the trajectory struct in MRIBase and the trajectory functionality in MRITrajectories. I currently don't see a clear need for that.

[aTrotier]

Thanks for the explanation. I also want to add that leeping all the package together is easier to maintain and tests. In the CI, we can directly see if the modification of a subpackage impact the reconstruction.

Like always I have to play with that on real example. For the MP2RAGE I should then only load :

• MRIReco (CS reco)
• MRIFiles (load Bruker dataset)
• MRICoilSensitivities (sensitivity maps)

One remarks: You have decided to totally remove the nifti dependency. But at some point we might need to add it back in order to fill the header appropriately. Maybe in a dedicated subpackage again ?

[tknopp]

One remarks: You have decided to totally remove the nifti dependency. But at some point we might need to add it back in order to fill the header appropriately. Maybe in a dedicated subpackage again?

Yes, this dedicated package is ImageUtils and I moved the nifti code there. You can also see this in the test dependencies. We actually still run the IO test: https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/master/test/testIO.jl which still works and saves / loads niftis. The only difference is that the two functions moved to ImageUtils and I made ImageUtils a test dependency. ImageUtils is a larger package and thus this reduces package load time.

[tknopp]

Update:

• I worked on making RegularizedLS independent of SparsityOperators and this worked out nicely. Also the handling of the normalOperator is now unified. One can see how this now looks like here: https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/master/src/Reconstruction/IterativeReconstruction.jl#L190. Most importantly the normal operator is now build outside of RegularizedLS. And for those who wonder why the normal operator is so special: It uses a Toeplitz optimization. We can further discuss making RegularizedLS independent of LinearOperators but let's move that discussion to RegularizedLS, not here.
• Now all packages are released and CI is green :-).
• I worked in documentation of https://github.com/MagneticResonanceImaging/MRIRecoBenchmarks. If one clicks now on the benchmarks, one can actually see the results and look where we stand. Making most of the operators non-allocating had a pretty significant impact. The Toeplitz operator is now super fast (https://github.com/MagneticResonanceImaging/MRIRecoBenchmarks/tree/master/benchmark1)

I will close this issue soon. If someone has opinions on the package splitting please test things out and look if that fits. It's not set in stone, packages can gradually evolve and be split or unified. The version that we have now seems to address the concerns that we had before and it is thus hopefully a step forward.

[aTrotier]

@tknopp Can we close the issue ? :)

[tknopp]

yes sure.