-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Adjoint for parent
for LowerTriangular
and UpperTriangular
#1444
Conversation
Ideally all of them should be moved to ChainRules, but that can be done in a future PR. Can you add a test? |
Added tests 👍 |
Are the test failures related to this PR? |
They do not appear to be, but we should fix them. Unfortunately I think that requires some coordination on the AbstractFFTs side and I have no idea what's going on there or with the FFT rules here. Perhaps @gaurav-arya or someone else in the know could help? |
Looks to be erroring on the FFT Chain Rules that have existed for quite a while in AbstractFFTs.jl, when an object of type Are objects of type |
OK, I see that So the error is indeed due to the |
Btw what's the release schedule like for Zygote? Would it be possible to make a new release somewhat soonish?:) |
Was hoping to cut a patch release after merging this PR, but I need to figure out whether the current failures on CI (ref. #1446) require a fix on the Zygote side to ensure we're not breaking anyone's code. However I am not the best person to investigate this since I have little knowledge of the interaction between the Zygote and AbstractFFT rules, so if anyone has more knowledge of this please chime in. |
Gotcha; thanks @ToucheSir 🙏 |
The errors are due to the CUDA fix in JuliaMath/AbstractFFTs.jl#105 and similar conversions for adjoint plans. These changes in AbstractFFTs broke compatibility of the rules with Zygote.OneElement. I don't think you can do anything in Zygote about it - unless you want to re-add the incorrect Zygote FFT rules. It has to be fixed downstream (there are open issues and PRs). |
Awesome; thanks! |
LowerTriangular
andUpperTriangular
is missing adjoints forparent
which can lead to downstream issues: TuringLang/Bijectors.jl#280 (comment)This PR adds those, similar to the existing for
Adjoint
andTransposed
.It's unclear to me whether this should actually go into ChainRules.jl or not 😕
I'll add tests in a bit.