AbstractFFTs.jl icon indicating copy to clipboard operation
AbstractFFTs.jl copied to clipboard

Add ForwardDiff extension

Open dlfivefifty opened this issue 1 year ago • 11 comments

This is moving type-piracy code from https://github.com/JuliaApproximation/FastTransformsForwardDiff.jl to an extension here.

dlfivefifty avatar Dec 12 '24 21:12 dlfivefifty

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.62%. Comparing base (70524d2) to head (4fe2464). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
ext/AbstractFFTsForwardDiffExt.jl 85.00% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
- Coverage   94.84%   94.62%   -0.22%     
==========================================
  Files           5        5              
  Lines         446      465      +19     
==========================================
+ Hits          423      440      +17     
- Misses         23       25       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 13 '24 09:12 codecov[bot]

@mikaelslevinsky do you know what the grammatrix FastTransforms failure is?

dlfivefifty avatar Dec 13 '24 09:12 dlfivefifty

This is closing in but will also need to add an extension to FFTW.jl.

@devmotion @jishnub any opinion on whether these extensions should live here/in FFTW.jl or both in ForwardDiff.jl?

dlfivefifty avatar Dec 13 '24 10:12 dlfivefifty

@mikaelslevinsky do you know what the grammatrix FastTransforms failure is?

Is this a case issue? 😳 The file clearly exists but with two capital letters.

MikaelSlevinsky avatar Dec 13 '24 14:12 MikaelSlevinsky

Every OS apart from MacOS is case-sensitive (I think)

dlfivefifty avatar Dec 13 '24 22:12 dlfivefifty

(I probably mean "every file system except AFS and its predecessors HFS and HFS+ but I'm probably a decade out of date 😅)

dlfivefifty avatar Dec 13 '24 22:12 dlfivefifty

Yeah it's fixed in 0.16.8

MikaelSlevinsky avatar Dec 13 '24 22:12 MikaelSlevinsky

Every OS apart from MacOS is case-sensitive (I think)

Nitpicking: file systems are case-sensitive/insensitive, not operating systems. Also, NTFS, most common file system for Windows, is typically set up to appear case-insensitive to users (even if internally it may behave differently).

giordano avatar Dec 13 '24 22:12 giordano

I made this PR to drop Julia <v1.10: https://github.com/JuliaMath/AbstractFFTs.jl/pull/140

Maybe we should merge that first and then I can make the suggested changes?

dlfivefifty avatar Dec 17 '24 22:12 dlfivefifty

I don't think you have to wait for that PR, you could remove the hard dependency on Julia < 1.9 right away.

devmotion avatar Dec 17 '24 23:12 devmotion

I suspect the complex case can be improved by using r2r with FFTW.R2HC on the real and imaginary part (after reinterpreting a dual array in the right way) but I that would be FFTW-only.

Note there is no support for in-place transforms: I don't know how to detect if a plan is in-place.

dlfivefifty avatar Dec 18 '24 11:12 dlfivefifty