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

MaternKernel AD #450

Open
willtebbutt opened this issue Apr 13, 2022 · 4 comments
Open

MaternKernel AD #450

willtebbutt opened this issue Apr 13, 2022 · 4 comments

Comments

@willtebbutt
Copy link
Member

#425 makes it possible to AD through the MaternKernel by dropping the derivative w.r.t. \nu. We tried ensuring that it returns a NotImplemented, but Zygote doesn't appear to handle it properly, returning a nothing, rather than NotImplemented.

@devmotion
Copy link
Member

Isn't that the convention in Zygote and expected? In Zygote every undefined gradient is nothing, so it seems that it would quite breaking if in some cases this convention would be violated.

To me it doesn't seem like a Zygote issue and rather nothing seems to be the correct result (regardless of whether one thinks that this nothing convention is a good thing or not - personally I think it's not 😄).

@willtebbutt
Copy link
Member Author

I'm okay (although disappointed) that this is what Zygote is doing. I think it's good to keep a record of this behaviour, so that we can a) point disgruntled users towards it, and b) point Zygote devs towards it if we feel so inclined.

@devmotion
Copy link
Member

Ideally, the convention in Zygote would be changed but I don't think this will happen anytime soon (and possibly never).

But it's still not clear to me why Zygote being surprising here should affect if we return NotImplented or NoTangent. Other CR ADs hopefully handle NotImplemented correctly, and in these cases the choice matters and the implementation in #425 seems wrong.

@devmotion
Copy link
Member

I'll take back my statement above. Apparently Zygote returns NotImplemented: FluxML/Zygote.jl#1204 (comment)

Possibly the issue in #425 was that the pullback was incorrect (missing NoTangent for the function itself) - but IIRC Zygote just uses nothing for missing derivatives.

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

No branches or pull requests

2 participants