JOSS review

[bwheelz36]

reviewing for: openjournals/joss-reviews#7743 (comment)

I was interested in reviewing this paper because of my experience when I was working an MRI distortion toolkit.
I had a lot of issues trying to convert between different nufft frameworks, all of which seemed to use slightly different formalism and language. this was really annoying, because they all have different strengths and weaknesses.

For this reason, I was quite excited to see this work, which promises to unify all these backends together.
this is a very rushed review, and I haven't had time to delve as deeply as I'd like to, but overall this seems like a really high quality package, with clear installation, good CI, and good documentation. I didn't quite manage to get it working with my code, but I'm pretty sure I could with a bit more fiddling :-P
overall, definitely think this should be published, and have raised a few issues below and in separate issues.

---

A few points I noticed while going through the code :-)

• why is CI failing on main branch?

• [here](https://mind-inria.github.io/mri-nufft/generated/autoexamples/example_2D_trajectories.html) I'm pretty sure the internal / external examples are the wrong way around. could be elsewhere too

• here the example doesn't run.

  • ok it's because I was using python 3.9, but your pyproject.toml suggests you support this
    • suggest using a matrix style to test all supported python versions, e.g. see [here](https://github.com/Image-X-Institute/TopasOpt/blob/cbb96dad4dc9c6f53e9b7ddc13ff185690ba54d8/.github/workflows/run_tests.yml#L15)
  • then had to install pooch, which isnt mentioned anywhere
  • then it runs but genreates a warning - why does the default example generate a warning?

• The below lines of code are a bit unconventional, in that I first get use get_operator to ... effectively import a class I think? and then below that I instantiate the class? Somehow in the end, what I get is an instance of mrinufft.operators.interfaces.finufft.MRIfinufft
  Unconventional isn't necessarily bad, but it's a bit hard to trace what's going on, and what it is that nufft IS. the docstrings are pretty good here, but one thing that would really help is more type hints.

  NufftOperator = mrinufft.get_operator("finufft")
  # For improved image reconstruction, use density compensation
  density = voronoi(samples_loc.reshape(-1, 2))
  # And create the associated operator.
  nufft = NufftOperator(
      samples_loc.reshape(-1, 2), shape=image.shape, density=density, n_coils=1
  )

[paquiteau]

Hello, thank for your interest, the kind words and your review of MRI-NUFFT !

I linked the related issue as sub-issue (new github functionality I am discovering, let's see how it goes)

why is CI failing on main branch?

it's quite new (CI was all green last week 🥲) and is likely because we use a local CI runner (for computation) see #242. Everything is green now 😊

[here](https://mind-inria.github.io/mri-nufft/generated/autoexamples/example_2D_trajectories.html) I'm pretty sure the internal / external examples are the wrong way around. could be elsewhere too

Could you please clarify what you mean by internal /external examples? The last link you provided is a general description of our interface, not an example (and therefore in our Explanation section of the documentation)

The below lines of code are a bit unconventional, in that I first get use get_operator to ... effectively import a class I think? and then below that I instantiate the class ?

Indeed, but we should update our example to be more straight-forward. The recommended usage would rather be:

nufft = get_operator("finufft", samples, shape, density="voronoi", n_coils=1) 

ksp = nufft.op(image) # get the k-space
image_adjoint = nufft.adj_op(ksp) # get the adjoint of the kspace

Which directly builds the operator and configures it (here to use "voronoi" density compensation, and a single coil)