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

SVD is broken in Zygote #205

Closed
MikeInnes opened this issue Jun 10, 2020 · 3 comments · Fixed by #207
Closed

SVD is broken in Zygote #205

MikeInnes opened this issue Jun 10, 2020 · 3 comments · Fixed by #207

Comments

@MikeInnes
Copy link

MikeInnes commented Jun 10, 2020

e.g.

gradient(x -> sum(svd(x).S), rand(10,10))

fails due to Zygote's NamedTuples not being supported by the rrule for this function.

Would be nice to have a type alias for Composite/NamedTuple so that rrules can be generic across both. Perhaps for now we can just remove the type restriction, since no other adjoint for SVD is possible anyway.

@nickrobinson251
Copy link
Contributor

Is this fixed by removing the restriction to Composite here?

function svd_pullback(Ȳ::Composite{<:SVD})

If so, I think removing that restriction is fine (likewise for the cholesky rrule).

Although i think we may also want to revisit adopting Composite in Zygote.jl FluxML/Zygote.jl#603

@MikeInnes
Copy link
Author

Yes, I believe that would do the job. I agree we should talk about that but obviously would love to fix the regression before having the design discussion.

@oxinabox
Copy link
Member

we don't need to support NamedTuple we need to support Composite{Any}

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 a pull request may close this issue.

3 participants