Ensure all fft-like functions fallback to version with region when region not provided

[gaurav-arya]

No description provided.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +2.88 🎉

Comparison is base (7d698db) 84.13% compared to head (bb29f03) 87.01%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
+ Coverage   84.13%   87.01%   +2.88%     
==========================================
  Files           2        2              
  Lines         208      208              
==========================================
+ Hits          175      181       +6     
+ Misses         33       27       -6     

Impacted Files	Coverage Δ
src/definitions.jl	74.03% <100.00%> (+5.76%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gaurav-arya]

@devmotion regarding documentation, the README already only asks that downstream implementations define AbstractFFTs.plan_fft(x, region), so I'm not sure if anything more is necessary?

[devmotion]

The code looks good to me. However, could we check that it does not break downstream packages? It seems we only test FFTW but IIRC there are other implementations of AbstractFFTs as well (might be good to add them to the integration tests?).

It seems in the documentation (e.g., https://juliamath.github.io/AbstractFFTs.jl/dev/) it is not mentioned that one should implement methods with region.

[gaurav-arya]

The instructions for defining a new implementation here completely omit how the plan should be formed, only what it should do. I think only the README discusses it. So the documentation could definitely be improved but it's out of scope for this PR? (I think I might have remedied it in the adjoint plans PR, don't recall, but in any case that might be a good place to do it.)

Regarding integration tests, I think CUDA and AMD define GPU FFTs, and FastTransforms and PencilFFTs both subtype AbstractFFTs.Plan and possibly rely on some other functionality. What do you think needs an integration test, and are GPUs an issue for tests?

[devmotion]

What do you think needs an integration test, and are GPUs an issue for tests?

Yes, that needs a different CI setup with buildkite. Maybe we could just check them locally for this PR? Usually it should not be required to test them. FastTransforms or PencilFFTs could be added to the integration tests, I think.

Regarding CUDA: I'm a bit surprised that they copy parts of AbstractFFTs - shouldn't https://github.com/JuliaGPU/CUDA.jl/blob/f9045bdf1213ff8ecfd418a74d655737c96ec0b1/lib/cufft/fft.jl#L269-L278 be covered by the generic definitions in AbstractFFTs?

[gaurav-arya]

I ran the test suite for FastTransforms locally and that one is fine. Unfortunately I don't have a GPU setup for testing CUDA/AMD. By inspection they look safe, but I don't have the ability to test for sure.

[gaurav-arya]

Regarding the CUDA implementation: Yeah, there seems to be a fair bit of code duplication in that file. I don't know, however, how much of it is simply redundant code and how much of it could be necessary hacks by downstream implementations to get past the various unresolved issues made in this repo. I'm tempted to keep the focus solely on the goal of supporting chain rules for now. I would like to work on refactoring to make sure AbstractFFTs makes life easy for downstream implementations in the future (starting with #32...), but for now I'm content with making sure nothing breaks.

[devmotion]

Yes, sure, improving CUDA is beyond the scope of this PR. The copied code only made me wonder whether this PR might break CUDA. Could we add FastTransforms to the integration tests? I'll try to test CUDA, I should have access to some GPUs.

[devmotion]

I ran the test/cufft.jl tests in CUDA with this PR, they passed successfully:

julia> include("cufft.jl")
Test Summary:  | Pass  Total   Time
T = ComplexF32 |   49     49  10.3s
Test Summary:  | Pass  Total  Time
T = ComplexF64 |   49     49  6.9s
Test Summary: | Pass  Total  Time
T = Float32   |   34     34  5.3s
Test Summary: | Pass  Total  Time
T = Float64   |   34     34  5.1s
Test Summary:      | Pass  Total  Time
T = Complex{Int32} |    2      2  8.4s
Test Summary:      | Pass  Total  Time
T = Complex{Int64} |    2      2  1.2s
Test Summary: | Pass  Total  Time
T = Int32     |    2      2  2.4s
Test Summary: | Pass  Total  Time
T = Int64     |    2      2  1.2s
Test Summary: | Pass  Total  Time
CUDA.jl#1268  |    1      1  6.6s
Test Summary: | Pass  Total  Time
CUDA.jl#1311  |    1      1  0.1s
Test.DefaultTestSet("CUDA.jl#1311", Any[], 1, false, false, true, 1.678232934992004e9, 1.678232935079531e9)

(jl_MyiIuu) pkg> st
Status `/tmp/jl_MyiIuu/Project.toml`
  [621f4979] AbstractFFTs v1.2.1 `../../home/davwi492/sources/AbstractFFTs.jl`
  [79e6a3ab] Adapt v3.6.1
  [ab4f0b2a] BFloat16s v0.4.2
  [052768ef] CUDA v4.0.1
  [7a1cc6ca] FFTW v1.6.0
  [0c68f7d7] GPUArrays v8.6.3

[gaurav-arya]

Could we add FastTransforms to the integration tests?

#86

[devmotion]

LGTM, local tests indicate it does not break downstream packages. I don't think we have to wait for #86.

[trahflow]

Thanks for your efforts, @gaurav-arya ! 🥇

Would you mind also doing a release?
I would then go ahead and drop the custom adjoints in Zygote

[gaurav-arya]

1.0 tests seem to have failed, maybe due to the ChainRulesCore weak dependency?? Will take a look asap

[devmotion]

No worries, I think this is harmless. There was a new version of ChainRulesTestUtils released today, and based on the logs I assume it added an isnothing check. Which does not work on 1.0.

[gaurav-arya]

I see, thanks

[vpuri3]

@gaurav-arya maybe we need something similar in FFTW for DCTs.