Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This error is only happening with the MKL provider. With MKL, FFTW.jl doesn't even compile on my machine. Could be due to 008bc5b? |
|
@devmotion, could you please review this? LMK if you want me to remove the |
devmotion
left a comment
There was a problem hiding this comment.
Can you explain the idea of the PR? The design goal was to define the differentiation rules in AbstractFFTs via adjoint plans. Downstream packages were supposed to implement this new adjoint interface.
|
Only the MKL tests are failing. Is there a regression with MKL? |
More concretely, #249 outlines the intended approach. FFTW was not supposed to define custom rules. |
|
This PR defines Chain Rules for |
|
With #249 , gradient computation would error for DCT/IDCT. This is because |
devmotion
left a comment
There was a problem hiding this comment.
Sorry, somehow I missed yesterday that the PR does not add rules for plans but for the dct, idct and r2r functions 🤦
As long as they/their interface is not moved to AbstractFFTs, rules should be defined here 👍
ext/FFTWChainRulesCoreExt.jl
Outdated
|
|
||
| # R2R | ||
|
|
||
| function ChainRulesCore.frule(Δ, ::typeof(r2r), x::AbstractArray, region...) |
There was a problem hiding this comment.
It seems the rrule for r2r is missing?
There was a problem hiding this comment.
The R2R transforms are not unitary. There is some scaling involved that depends on the kind of R2R transform. Because it looks like an involved task, I chose to skip that for now. I am happy to look into that in a separate PR
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
|
@devmotion I've addressed all your comments. LMK if you have more questions :D |
|
@devmotion ping :) |
address #272