drop adjoints for [i,r,b]fft() #1386
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Partially addresses FluxML#1377 ChainRules for these have been added in JuliaMath/AbstractFFTs.jl#58
|
I've made #83 to handle the chain rules issue you noted! |
|
Thanks! |
trahflow
commented
Mar 8, 2023
| @@ -27,7 +27,7 @@ Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2" | |||
| ZygoteRules = "700de1a5-db45-46bc-99cf-38207098b444" | |||
|
|
|||
| [compat] | |||
| AbstractFFTs = "0.5, 1.0" | |||
| AbstractFFTs = "1.3.1" | |||
There was a problem hiding this comment.
should this be done?
There was a problem hiding this comment.
Bumping the compat version of a dependency to add a new min bound? That's perfectly fine as long as we're not suddenly breaking anything. We do it all the time for e.g. Flux -> NNlib.
|
I think the build error is unrelated (and will be fixed by #1387 ) |
ToucheSir
approved these changes
Mar 8, 2023
Co-authored-by: Brian Chen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Following the discussion in #1377 I'm dropping the adjoints for
fft(),ifft(),rfft()etc.since the rules for
rfft()andirfft()were not correct and correct rules have since been added toAbstractFFTsin JuliaMath/AbstractFFTs.jl#58.Note that this does not yet fully fix the issue, since the adjoints for multiplication/left division by a real fft
AbstractFFTs.Planare still incorrect.There is an in-progress PR at
AbstractFFTsto provide ChainRules for these:JuliaMath/AbstractFFTs.jl#67
For now, the adjoints for multiplication/left divison by
AbstractFFTs.Planare kept, even though they lead to incorrect results for real forward and inverse ffts.Note though that if the
dimsargument is not provided tofft()etc., the rules provided by JuliaMath/AbstractFFTs.jl#58 aren't actually hit, and the compilation will fail trying to differentiate a try/except block inAbstractFFTs.plan_fft()This is still better than a silently incorrect result as in
rfft()andirfft(), but a regression forfft()etc. (without thedimsargument) where the rules defined in Zygote were seemingly correct.(which is why I made this a draft. Opinions on this welcome!)
PR Checklist
[ ] Documentation, if applicable