support functors in at-non_differentiable#298
Conversation
simeonschaub
force-pushed
the
sds/nondiff_functors
branch
from
e8db527 to
3375536
Compare
src/rule_definition_tools.jl
Outdated
| :function, | ||
| Expr(:call, propagator_name(primal_name, :pullback), :_), | ||
| Expr(:tuple, NO_FIELDS, Expr(:(...), tup_expr)) | ||
| Expr(:tuple, isfunctor ? DoesNotExist() : NO_FIELDS, Expr(:(...), tup_expr)) |
There was a problem hiding this comment.
Ratherr than passing in isfunctor to do this in the macro defintition
could we do something in the body that compiles away like check T<:Function && fieldcount(T) == 0?
There was a problem hiding this comment.
I don't think that check would work well for types, since e.g. DataTypes do have fields, but we don't care about the fields for the purposes of Zygote.
Of course, we could also just always return DoesNotExist() here, so @non_differentiable would just always imply that the function argument has no well-defined derivative, where it's not really important whether it actually has fields or not.
There was a problem hiding this comment.
Yeah, we could just return DoesNotExist for all.
Things without fields are not pertubable so DoesNotExist is not invalid for them.
(Zero is also valid since there is no errror for perturrbing them since it is impossible to pertube them)
I think i would be down with that change.
I don't think it would be breaking since nothing checks types of AbstractZero, just that that are zero.
There was a problem hiding this comment.
We do technically document that the derivative for the primal itself is NO_FIELDS, but I agree with your assessment that it's unlikely to break any actual code. Would you say that's still fine for a patch release?
There was a problem hiding this comment.
Bump. Since this is holding up JuliaDiff/ChainRules.jl#358
simeonschaub
force-pushed
the
sds/nondiff_functors
branch
from
3375536 to
2eec0d7
Compare
mzgubic
left a comment
mzgubic
left a comment
There was a problem hiding this comment.
Thanks for the PR, looks good to me. Some minor style comments added.
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
==========================================
- Coverage 89.39% 87.87% -1.52%
==========================================
Files 13 13
Lines 462 429 -33
==========================================
- Hits 413 377 -36
- Misses 49 52 +3
Continue to review full report at Codecov.
|
simeonschaub
added
needs version bump
rule definition helper
simeonschaub
force-pushed
the
sds/nondiff_functors
branch
from
42091e2 to
60e72c9
Compare
* fix tests for JuliaDiff/ChainRulesCore.jl#298 * Apply suggestions from code review Co-authored-by: Lyndon White <oxinabox@ucc.asn.au> Co-authored-by: Lyndon White <oxinabox@ucc.asn.au>
4 participants
No description provided.