Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current adjoint for FFT plans in Zygote doesn't handle
ScaledPlan
s. 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 aScaledPlan
rule, and adds more comprehensive tests which explicitly test every possible forward and inverse plan (and therefore test for theScaledPlan
issue that was not being handled previously).Also, it seems like backwards rules for
fft
s are being moved intoAbstractFFTs.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 ingradcheck.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 inAbstractFFTs.jl
.