-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Duplicate implementation of power iteration #109
Comments
Hmm, I agree with your general intend, but it would further increase the dependencies? I would still prefer to move all coil estimation code out of MRIReco.jl into a dedicated, light weight package and in this case it would be nice to not introduce a dependency for this simple function. But I am interpreting @tknopp's silence on this topic as a preference of his to keep the code in MRIReco.jl (which is of course fair enough). Am I interpreting this correctly, @tknopp? |
I am actually not sure what the right modularization approach is here. Would you really see MRICoilSensitivities to be completely independent of any image reconstruction algorithm? |
Yes, I would. This way, you could easily use it any other package w/o heavy dependencies. Just to give you one (egocentric) use example: I am currently setting up a MRFingerprintingRecon.jl package that will eventually implement a low rank ADMM algorithm. I think this is too specific to fit into something like MRIReco.jl, but, of course, it heavily builds on RegularizedLeastSquares.jl, NFFT.jl etc. And I include MRIReco.jl, purely for ESPIRIT. From my point of view, a package like MRICoilSensitivities.jl provides tools on the same abstraction level as, e.g. NFFT.jl and such a setup would streamline the implementation of specific image reconstruction packages. |
I too would like the convenience of a separate, smaller package for using ESPIRIT or such. |
Jeff made my point much more clear. If we put But I am all for proper modularization so it would be very helpful, if you two could give me some sketches on how your ideal package structure (as a tree) looks like. Then we can decide if we should name it @JakobAsslaender: It would be awesome if we could group all MR packages under the same Github organisation (MagneticResonanceImaging). You might consider moving MRFingerprintingRecon.jl. This would give us a much more clear picture what Julia packages we have and how we should group them properly. Same goes for BART.jl as I recently said. I feel that this gives |
Thinking more, perhaps I muddied the water too much. JSENSE is a recon method that happens to also produce coil sensitivities, so arguably it belongs in MRIreco. I would prefer |
I agree with @JeffFessler, JSENSE is foremost an image reconstruction and should be part of MRIReco.jl. The implementation of ESPIRIT, with a main function that takes an array of numbers as input. The wrapper function that takes the MRIReco.jl data format would stay in MRIReco.jl, and if someone were to introduce a new data format for whatever reason, they could write their own wrapper in their package. And yes, I will move the packages. I just need to figure out how to do that w/o breaking the registry etc. |
I will have a look into this when I find some time. |
There is, by the way, yet another power iterations implementation :-) I would like to keep the duplicated code for the moment and first do the |
I think I found a 4th implementation :-) https://iterativesolvers.julialinearalgebra.org/stable/eigenproblems/power_method/ I would argue that we should just use that and remove all other implementations. |
I made some small tests and it seems the method in If there is a performance issue in IterativeSolvers, we should just fix that there and afterwards remove our implementation. |
I just stumbled upon the fact that we have one method
power_iterations!
in MRIReco (which in turn is used by espirit) and a similar methodpower_iterations
in RegularizedLeastSquares. The only difference between these methods seem to be the default values forrtol
andmaxiter
and the fact thatpower_iterations!
allows to pass a pre-allocated vector forb
.I think it would be nice to adapt the implementation in RegularizedLeastSquares so that it can be used in espirit and then remove the implementation in MRIReco. I think it should not be a great deal of work and we would avoid this little bit of duplicate code.
What do you think about this @JakobAsslaender?
The text was updated successfully, but these errors were encountered: