Skip to content

Conversation

@nHackel
Copy link
Contributor

@nHackel nHackel commented Sep 1, 2025

Hello @jipolanco,

over at MRIOperators we discussed a limitation of AbstractNFFTs interface. Mainly, its inability to allow for both NFFT and NonuniformFFTs (or any second package) to be loaded at the same time in such a way that a user can chose which NFFT provider/package to use.

I've provided a potential solution to this problem over at this PR and I've implemented the necessary changes for NonuniformFFTs in this PR. I'd be happy to hear your feedback to this approach

@nHackel nHackel marked this pull request as draft September 2, 2025 06:37
@jipolanco
Copy link
Owner

Hi @nHackel, thank you for proposing these changes to the AbstractNFFTs interface. I'd be happy to make the required changes to adapt to the new interface.

I raised the same issue you mention some time ago, and I think what you propose based on backends is a nice and elegant solution to this problem. If you don't mind, I'll wait for JuliaMath/NFFT.jl#149 to be merged and for AbstractNFFTs.jl v0.9.0 to be released before merging this PR in some form.

As a minor comment, I'd replace NonuniformFFTBackend with NonuniformFFTsBackend (note the extra s) for consistency with the name of this package.

@nHackel nHackel marked this pull request as ready for review November 7, 2025 13:49
@nHackel
Copy link
Contributor Author

nHackel commented Nov 7, 2025

Hello @jipolanco, I've finally had some time to merge and release AbstractNFFTs v0.9 with the proposed changes and I think this PR is now ready for review

@nHackel
Copy link
Contributor Author

nHackel commented Nov 7, 2025

@jipolanco would it be possible to add the precompute keyword argument here.

Something like:

Base.@constprop :aggressive function NFFTPlan(
        xp::AbstractMatrix{T}, Ns::Dims;
        # working keyword arguments ...
        precompute = nothing,
        kws...,
    ) where {T <: AbstractFloat}

if !isnothing(precompute)
  @warn "Precompute flags are not yet supported and will be ignored"
end

I'm currently propagating the AbstractNFFTs changes up to MRIReco.jl and I'm testing to see if one can just use NonuniformFFTs as a drop-in replacement. So far I am failing on the keyword argument.

I can gladly make a seperate PR for all the changes I'd need for that and then we can decide seperately from this PR

@codecov
Copy link

codecov bot commented Nov 7, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.66%. Comparing base (a4b957e) to head (2ee6edd).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/abstractNFFTs.jl 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master      #68   +/-   ##
=======================================
  Coverage   89.66%   89.66%           
=======================================
  Files          24       24           
  Lines        1838     1839    +1     
=======================================
+ Hits         1648     1649    +1     
  Misses        190      190           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jipolanco
Copy link
Owner

@jipolanco would it be possible to add the precompute keyword argument here.

Yes, that would be fine. I would just slightly change the warning message to something like "Precompute flags are not supported by the NonuniformFFTs backend and will be ignored", since I don't think NonuniformFFTs will ever support a precompute option. I will include it in this PR to make things simple.

@jipolanco jipolanco merged commit 82213a7 into jipolanco:master Nov 7, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants