-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add NotImplemented and NotImplementedRule #28
Conversation
Sometimes you just don't implement something. Maybe it's hard, maybe you're lazy like me, whatever. For such cases, there is the differential `NotImplemented` and its associated rule `NotImplementedRule`. This was born of not being able to figure out the rule for the scalar multiple parameter in `BLAS.gemm`, but having implemented rules for the matrix parameters.
NotImplemented <: AbstractDifferential | ||
|
||
A differential type which behaves similar to [`DNE`](@ref) but instead signifies that | ||
the actual differential is not implemented in ChainRules, not that it does not exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can there be a "not yet" added here?
Maybe even a "PRs to remove not implemented rules are welcome"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
The more I think about this, the less I like it. |
Seems fine. Seems like it might be useful. Shall we do this? |
Up to you/whoever maintains this now. I won't likely have time to revisit this but feel free to do what you will with the branch. |
Yes, I want to return this instead of Nothing as the default fallback |
""" | ||
NotImplementedRule <: AbstractRule | ||
|
||
Rule indicating that a particular derivative is not implemented by ChainRules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with @oxinabox 's comment above, I would like to see this expanded slightly to say more about what a NotImplementedRule
does mean. Some context would be helpful e.g.
"""
Rule indicating that a particular derivative is not yet implemented by ChainRules. Different from [`DNERule`](@ref) in that this rule does not imply non-differentiability, but rather that no one has implemented it yet.
"""
I'm very happy for this to be merged provided that someone addresses the docstrings to provide a bit more context for the confused rule-implementer. |
Cool, I can take a look at finishing this up next week, unless someone else is interested in doing it :) |
For context currently we do: The AD system when it gets that back, know that it has to actually go and do AD. I was proposing we change that to: Advantage of this is internal consistancy, and less magic This case does still show up for case where 1 of the derivatives is listed as |
I had a discussion with @willtebbutt about this. And we concluded that for now we don't need it, I am closing this PR, but we can reopen it later when/if we need it. |
Sometimes you just don't implement something. Maybe it's hard, maybe you're lazy like me, whatever. For such cases, there is the differential
NotImplemented
and its associated ruleNotImplementedRule
.This was born of not being able to figure out the rule for the scalar multiple parameter in
BLAS.gemm
(#25), but having implemented rules for the matrix parameters. As indicated by the previous paragraph, I am lazy; it is easier to add a rule that admits that to the world than it is to actually figure out how to differentiate wrt alpha.