Fix method ambiguity with FFTW.jl for plan_rfft/plan_brfft - #131
Open
pankgeorg wants to merge 1 commit into
Open
Fix method ambiguity with FFTW.jl for plan_rfft/plan_brfft#131pankgeorg wants to merge 1 commit into
pankgeorg wants to merge 1 commit into
Conversation
With FFTW.jl loaded alongside FFTA, plan_rfft(::Vector{Float64}, ::Int) was
ambiguous between FFTW's StridedArray method and FFTA's method annotated
with region::RegionTypes, turning rfft(x) into a MethodError. Leave region
unannotated on the AbstractFFTs entry points (as plan_fft already does) and
normalise it in an internal function, so FFTW's methods are strictly more
specific and take over as AbstractFFTs intends.
A coexistence test runs in a subprocess (loading FFTW in the test process
would make every other test exercise FFTW).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
==========================================
- Coverage 98.80% 98.64% -0.17%
==========================================
Files 5 5
Lines 585 589 +4
==========================================
+ Hits 578 581 +3
- Misses 7 8 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With FFTW.jl and FFTA loaded in the same session,
plan_rfft(::Vector{Float64}, ::Int)— and thereforerfft(x)— is a method ambiguity:FFTW's method is more specific in the array argument, FFTA's in the region argument, so neither wins. Same for
plan_brfft(d::Integervslen::Int).plan_fft/plan_bfftwere fine because their region argument is unannotated.This leaves
regionunannotated on theAbstractFFTs.plan_rfft/plan_brfftentry points (asplan_fftalready does) and normalises it in an internal_plan_rfft/_plan_brfft, so that FFTW'sStridedArraymethods are strictly more specific and take over when loaded — which is what AbstractFFTs intends (ref #66 for the discussion of that behaviour). FFTA's methods remain reachable for non-strided arrays.A regression test loads FFTW.jl and checks
rfft/irfft/fftwork and that noplan_*ambiguities exist. It runs in a subprocess, becauseusing FFTWin the test process would make every other test in the suite exercise FFTW instead of FFTA. This adds FFTW as a test-only dependency; happy to drop the test if that's unwanted.Found while writing the benchmark suite in #128, where FFTA and FFTW are loaded together.