search

MagneticResonanceImaging / MRIReco.jl

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

Propagating arbitrary Type compatibility throughout codebase #71

Closed alexjaffray closed 2 years ago

alexjaffray commented 2 years ago

Continuing with the idea brought forward with PR #67 in adding arbitrary Type compatibility into MRIReco.jl, I have propagated the convention of using parametric types into the majority of the off-resonance code.

The tests all pass on Mac and Linux, with the exception of a reconstruction equality check in testISMRMRD.jl. I have commented out the failing test for now as it seems to be a precision issue, but need to investigate whether the cause of the failure is a precision error or something more subtle.

There are some areas of uncertainty for me in pushing this further on my own, mainly with regards to ensuring the inputs to the NFFT package calls are correct. I would appreciate help in this regard if you have some time to review this.

Thanks! I am really appreciating how active this repo is in the recent few months, it's very motivating!

tknopp commented 2 years ago

This looks very good! Not clear to me why the ISMRMRD test should fail since you have not done anything touching that code.

tknopp commented 2 years ago

I added you to the MagneticResonanceImaging group so that you should have write access to MRIReco.jl. You can then create branches in MRIReco.jl directly (same holds true for you @aTrotier). You can still make PR but the huge advantage is that I can directly work on your branch and make smaller corrections before merging.

Still merging this now and fix some smaller stuff in a following commit.

tknopp commented 2 years ago

the ISMRMRD test passes for me locally. I disabled the other tests though to test this in an isolated way. Currently make a full run.

aTrotier commented 2 years ago

Great

I was also working on that yesterday. Maybe we should start to have a roadmap in order to not duplicate the development between us.

alexjaffray commented 2 years ago

Thanks for adding me @tknopp!

I agree with @aTrotier, I think it would make sense to do some planning in this regard. A formal roadmap is a good idea to split up potential work in an efficient manner. This would allow us to tackle some of the ideas brought up in #66 and also to make sure that the scope and spec of feature additions is well-defined.

In the interim, we could indicate in the title if a PR will be a work in progress or more straightforward (addition of functionality vs. bugfixes for example). Not sure what the best approach is ... Thoughts?

tknopp commented 2 years ago

Using branches and WIP: ... in the title is best practice and we should do exactly that.

Roadmap is a good idea, but I have to say that things are evolving somewhat dynamically right now. Smaller things can be solved by small PR and merging quickly to master. For larger things you should simply start issues and indicate on what you want to work on.

The most important point for me is to shape the Julia MR packages into a direction that other research can use it for actual (research) work. Since I do not work on MR exclusively I appreciate if other full-time MR researchers join and push those things forward where MRReco is still lacking functionality.