Skip to content

Conversation

vpuri3
Copy link

@vpuri3 vpuri3 commented Jul 5, 2023

The potentially incorrect Zygote rules for FFT (#899) can be removed now that comprehensive Chain Rules have been added in JuliaMath/AbstractFFTs.jl#67

cc @devmotion

PR Checklist

  • [ ] Tests are added
  • [ ] Documentation, if applicable

The potentially incorrect Zygote rules for FFT (FluxML#899) can be removed now that comprehensive Chain Rules have been added in JuliaMath/AbstractFFTs.jl#67
@devmotion
Copy link
Collaborator

devmotion commented Jul 5, 2023

I assume they can't be removed right now since the AbstractFFT PR is not sufficient - downstream packages have to implement the adjoint functionality first. Otherwise the rules will just error.

@ToucheSir ToucheSir added the ChainRules adjoint -> rrule, and further integration label Jul 5, 2023
@devmotion
Copy link
Collaborator

Indeed, tests fail currently since e.g. AbstractFFTs.ProjectionStyle is not defined for FFTW plans.

@piever
Copy link
Contributor

piever commented Sep 19, 2023

Sorry to just bump this, but I've also run into #899 and wanted to check whether this can be solved. As JuliaMath/FFTW.jl#249 has been merged, would it be possible to tag a patch release of FFTW and merge this?

@vpuri3
Copy link
Author

vpuri3 commented Jan 18, 2024

rerunning CI

@vpuri3
Copy link
Author

vpuri3 commented Jan 18, 2024

@stevengj, @devmotion could you do this?

Sorry to just bump this, but I've also run into #899 and wanted to check whether this can be solved. As JuliaMath/FFTW.jl#249 has been merged, would it be possible to tag a patch release of FFTW and merge this?

@gaurav-arya
Copy link

For tests to pass with this PR, we also need an AbstractFFTs release due to JuliaMath/AbstractFFTs.jl#116. In addition, the (half incorrect) Zygote rules that would be removed here do apply to GPU code, while support for AdjointStyle's by CUDA is bottlenecked by JuliaMath/AbstractFFTs.jl#118.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChainRules adjoint -> rrule, and further integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants