Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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.

Sign up for GitHub

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

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

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

wants to merge 1 commit into 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

@vpuri3
Remove Zygote rule for *(::AbstractFFTs.Plan, ::AbstractArray)
b1c1a6e 
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 •
edited
Loading

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.

@gaurav-arya gaurav-arya mentioned this pull request Jul 9, 2023
Open
5 tasks
@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?

@awadell1 awadell1 mentioned this pull request Jan 17, 2024
Closed
@vpuri3 vpuri3 closed this Jan 18, 2024
@vpuri3 vpuri3 reopened this Jan 18, 2024
@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?

@stevengj stevengj mentioned this pull request Jan 18, 2024
Merged
@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
Reviewers
No reviews
Assignees
No one assigned
Labels
ChainRules adjoint -> rrule, and further integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vpuri3 @devmotion @piever @gaurav-arya @ToucheSir
Morty Proxy This is a proxified and sanitized view of the page, visit original site.