Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ForwardDiff extension #138

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Add ForwardDiff extension #138

wants to merge 19 commits into from

Conversation

dlfivefifty
Copy link
Member

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

@dlfivefifty dlfivefifty changed the title Add ForwardDiff extension WIP: Add ForwardDiff extension Dec 12, 2024
Copy link

codecov bot commented Dec 13, 2024

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 ⚠️
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     

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

@dlfivefifty
Copy link
Member Author

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

@dlfivefifty
Copy link
Member Author

dlfivefifty commented Dec 13, 2024

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?

Project.toml Outdated Show resolved Hide resolved
@MikaelSlevinsky
Copy link

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

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

@dlfivefifty
Copy link
Member Author

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

@dlfivefifty
Copy link
Member Author

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

@MikaelSlevinsky
Copy link

Yeah it's fixed in 0.16.8

@giordano
Copy link
Member

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).

src/AbstractFFTs.jl Outdated Show resolved Hide resolved
src/AbstractFFTs.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@dlfivefifty
Copy link
Member Author

I made this PR to drop Julia <v1.10: #140

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

@devmotion
Copy link
Member

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

@dlfivefifty dlfivefifty changed the title WIP: Add ForwardDiff extension Add ForwardDiff extension Dec 18, 2024
Co-authored-by: David Widmann <[email protected]>
@dlfivefifty
Copy link
Member Author

dlfivefifty commented Dec 18, 2024

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.

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.

5 participants