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

Remove Zygote rule for *(::AbstractFFTs.Plan, ::AbstractArray) #1437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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