MPoL-dev / MPoL

A flexible Python platform for Regularized Maximum Likelihood imaging
https://mpol-dev.github.io/MPoL/
MIT License
33 stars 11 forks source link

splitting NuFFT into base and uu,vv cached class. #232

Closed iancze closed 9 months ago

iancze commented 9 months ago

closes #231 when complete.

iancze commented 9 months ago

Thanks for the review!

  • It looks like the pyro tutorial has an outdated called to the nufft.

Ah, right. This is in the "large-tutorials" whose cache is not cleared with a make clean, so I missed it.

  • The naming for nufftcached and nufft makes sense, but I would suggest not reusing the name of an old class for a new class with a different functionality and/or call signature. Yes the code is pre-1.0 and you want the new nufft to be the default, but this breaks backwards compatability and will probably be frustrating for some users. Instead, the cached implementation could still be NuFFT and the new could be NuFFTdynamic or something. If the new names are kept as is, I'd suggest the changelog note that the NuFFT is not backwards compatible as of the next release version.

I agree the change of names is unfortunate, but I think the original implementation and naming was enough of a mistake that it should be corrected (especially since we are pre-1.0, the user base is small, and we should take these opportunities for re-organization/consolidation). The new NuFFT should be the preferred behavior in most cases (and more closely matches the original TorchKbNuFFT pattern), whereas the caching behavior is much more specialized. Honestly, for ALMA-sized datasets, I'm not even sure if there will be many cases where NuFFT cached will be used. It's prohibitively slow as the number of visibility points grows, so predicting full datasets will be necessary using the behaviour of the new NuFFT. I'll make an entry in the changelog about the switch.