MagneticResonanceImaging / MRIReco.jl

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

MRIFieldmaps.jl (working title) #132

Closed tknopp closed 1 year ago

tknopp commented 1 year ago

As discussed in the last community call, we are missing proper field map estimation tools right now. This is a little bit unfortunate since fieldmap aware reconstruction is one of the core strengths of MRIReco.jl.

@alexjaffray has unpublished code for field map estimation (based on which paper?) and I created an empty package (https://github.com/MagneticResonanceImaging/MRIReco.jl/tree/master/MRIFieldmaps) where it can be put in.

Regarding the scope my feeling would be that we should consider the fieldmap to be complex-valued, i.e. both off-resonance and relaxation are taken into account. I.e. we could also put tools in here that estimate relaxation maps based on longer echo trains.

The package does not need to be registered immediately so the scope and the name are open for discussion. Right now, the main purpose is to give the code of @alexjaffray a home. We shortly discussed creating a package MRIUtils.jl where all the things go in that don't fit into the other packages but I hesitate a little creating a package without a defined scope.

Alex: To get started please read the README.md where it is explained how to dev the package.

alexjaffray commented 1 year ago

@tknopp Thanks for getting this started! I'll read the readme and push my work later today. The functions are already documented so this should be straightforward, and the MRIFieldmaps.jl package-to-be is likely a good starting point for gathering more fieldmap estimation routines (including relaxation) into the MRIReco suite.

The functionality of my code is a (admittedly quite) naïve implementation of the 2008 paper "Regularized Field Map Estimation in MRI" (https://doi.org/10.1109/TMI.2008.923956) from the group of @JeffFessler. Right now the gradient of the cost function is performed using Zygote (AD) and is fast enough for my purposes but explicit gradient calculation would be a welcome improvement.

JeffFessler commented 1 year ago

Sorry I missed the last call. To atone for my oversight, I am working on a Julia translation of our 2020 version https://github.com/ClaireYLin/regularized-field-map-estimation which we found to be much faster than the 2008 paper - in Matlab. I think it is great that @alexjaffray is working on the 2008 version because I have not converted that one to Julia and it will be neat to verify that the speedups we saw in Matlab also hold in Julia. I'll post again when I have some code debugged...

JeffFessler commented 1 year ago

It's great to see the first version in #133. I have finished translating my version from Matlab into Julia and it runs but needs debugged - didn't work first try 😢 Features it will have include both 2D and 3D images in a unified code, 2 or more echo times, and multiple coils. @alexjaffray's cost function should be a special case which will be good for double checking things.

My code will not provide R2 or R2* map estimates at this point. Probably separate optimizers are needed for that. But it could be fine to have the repo name be general enough to include that. Sensitivity maps (receive B1 maps) and transmit maps (B1+ maps) are all field maps too. OTOH, R2 maps are not magnetic fields and there is the whole MRF field with its own techniques...

I would like to propose a way to organize things. At the org level it would be nice to have a streamlined repo that provides methods that do field mapping, just working with array inputs (ala @JakobAsslaender 's preferences too i think) with minimal dependencies. Then within MRIreco there can be an interface to those methods that uses all the MRIreco data types etc.

alexjaffray commented 1 year ago

@JeffFessler thanks for taking a look, I'll incorporate your suggestions and commit shortly. It's great that there is synergy on this, our work depends on good field maps so I'm excited to try your translated version.

As far as R2* estimation goes, @kamesy has implemented a few common methods in Julia in the utils folder (src/utils/r2star.jl) of his package https://kamesy.github.io/QSM.jl. They work with array inputs so they most likely are easily interfaced to MRIReco in a way that is consistent with yours and @JakobAsslaender 's preferences.

aTrotier commented 1 year ago

I also have some simple function for T2/T2* mapping (and MP2RAGE) here : https://github.com/aTrotier/qMRI.jl/

tknopp commented 1 year ago

I am right now not deep enough in all these mapping things to:

  1. judge if all these methods fit into a single coherent package
  2. structure, and maintain that package So it would be great if some of you could work on this. If it is preferred, we can move MRIFieldmaps.jl to its own Github repo but this implies that someone should feel (at least a little) responsible for it.
aTrotier commented 1 year ago

It is not related to the reconstruction part of MRIReco so it makes sense to have a dedicated package which will be easier to handle and also have more visibility for potential users.

My initial idea with qMRI.jl was also to create something like qMRLab and can be used for post-processing which works on arrays, I don't know if all the function are in the scopes of MRIFieldmaps.jl

JeffFessler commented 1 year ago

I'd be glad to maintain an independent MRIFieldmaps package that focuses on B0 initially and later B1. I'd prefer to leave the broader qMRI ecosystem to others at this point because it's probably too big to put in one package. I'm still debugging. When I get something working I'll package it up and ask for reviews and eventually it can be moved to MRI org.

JeffFessler commented 1 year ago

Ok, I now have a working version at https://github.com/MagneticResonanceImaging/MRIfieldmaps.jl Unless there are objections, I plan to register this version (0.0.1) because I want to have an archive of it in this state because it kind of reproduces the results in our 2020 paper. Then I have plans to improve the code factoring, and extend the algorithm in ways that depart from that paper, and I am sure it will evolve with input from others too. It would be great to have comments on what I've done so far.

JeffFessler commented 1 year ago

I'd propose we close this issue here and report any issues with the code in the new repo, but I leave it to you @tknopp since you opened the issue.

JeffFessler commented 1 year ago

I've started the registration process because it takes 3 days. So if you have objections to my registering v0.0.1 please speak up within 3 days! https://github.com/JuliaRegistries/General/pull/72696

mrikasper commented 1 year ago

@JeffFessler That's great, thank you for pushing this forward, I will definitely test this out!

I have a small, but maybe relevant question: Do we have any naming conventions in our (MRI) Julia community for new packages (or rather: their GitHub repo name), in particular with respect to capitalization and the .jl extension? Most of the sub-packages in MRIReco.jl follow the convention MRI<X>xxxxx, while the package itself is that plus the .jl extension. now your new repo is MRI<x>xxxxxx.jl. I am fine with either, but it might lead to the occasional error when trying to use packages. Or is using in Julia case-insensitive?

Personally, I really like the convention of adding .jl to the repo names so they stand out as Julia packages. With respect to camel-case or not, I think I would propose to decide on what needs the fewest changes.

JeffFessler commented 1 year ago

Ahh, I hadn't thought about that. I am fine with the package name MRIFieldmaps and repo name MRIFieldmaps.jl, which would be consistent with package MRIReco and repo name MRIReco.jl. Is that what you would prefer? (To my eyes having a capital letter right after an acronym looks funny, but I agree that consistency with the other packages in the org is preferable.) I have paused the registration to give a chance for at least one more person like @tknopp to weigh in! Let me comment that having .jl here is consistent with the org-level name of MRIReco.jl. (It is only the "sub-packages" within MRIReco that are "missing" the .jl extension and it's fine to omit it there because it is within a Julia repo.)

tknopp commented 1 year ago

Yes, I would prefer MRIFieldmaps as the package name. Mainly because of consistency. Acronyms and camel case is always a little bit odd. But look for instance at https://github.com/SciML, which is one of the largest Julia github organization where they also use names like SciMLBenchmarks.jl. (or https://github.com/JuliaGPU has GPUArrays.jl)

Regarding the .jlpostfix in the Github repo names there is the clear recommendation to use it. The idea is that one can directly identify Julia packages just by looking at the repository name. This is in particular nice for packages like CUDA.jl or FFTW.jl which are wrappers around C libraries. Sub-packages don't require .jl and I have never seen a sub-package using that.

These conventions are not really org-level conventions but rather Julia package ecosystem conventions. Unfortunately I cannot find a link where the conventions are formalized.

JeffFessler commented 1 year ago

Agreed. Done.

tknopp commented 1 year ago

I am closing this now. Further discussions can happen at the new Github repo.