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

ScaledPlan backwards rule #1173

Closed
wants to merge 2 commits into from
Closed

ScaledPlan backwards rule #1173

wants to merge 2 commits into from

Conversation

gaurav-arya
Copy link

@gaurav-arya gaurav-arya commented Feb 23, 2022

The current adjoint for FFT plans in Zygote doesn't handle ScaledPlans. See for example:

JuliaMath/FFTW.jl#182
SciML/OperatorLearning.jl#11

This issue mostly arises because the inverse plan of e.g. an FFT makes use of ScaledPlan. This rule adds a ScaledPlan rule, and adds more comprehensive tests which explicitly test every possible forward and inverse plan (and therefore test for the ScaledPlan issue that was not being handled previously).

Also, it seems like backwards rules for ffts are being moved into AbstractFFTs.jl using ChainRules. However, there are no backwards rules there for FFT plans yet, see JuliaMath/AbstractFFTs.jl#63. (The same issue holding that back -- that the type of an FFT plan does not convey enough information for the backwards rule for real FFTs -- leads to a few errored tests in Zygote, which are manually skipped in gradcheck.jl). But until that happens, it seems like the rules in Zygote are the only ones that handle FFT plans to at least some extent, so this patch should be helpful until plans are properly handled in AbstractFFTs.jl.

@gaurav-arya gaurav-arya marked this pull request as ready for review February 24, 2022 00:57
@devmotion
Copy link
Collaborator

Just a heads up: The rules for rfft etc. are incorrect in Zygote but implemented correctly in AbstractFFTs (#1158).

@gaurav-arya
Copy link
Author

I see. I think that this PR, which partially handles FFT plans, still make sense, do you agree? But perhaps this PR / a separate PR should get rid of all the non-plan FFT rules in Zygote, what do you think?

@gaurav-arya
Copy link
Author

Closing because we should hopefully have ChainRules for plans somewhat soon, after which this rule can just go.

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.

2 participants