Port a few rules from Diffractor #463
Conversation
| @@ -98,27 +98,31 @@ end | |||
| Adjoint_mat_pullback(ȳ::Tangent) = (NoTangent(), ȳ.parent) | |||
| Adjoint_mat_pullback(ȳ::AbstractVecOrMat) = (NoTangent(), adjoint(ȳ)) | |||
| Adjoint_mat_pullback(ȳ::AbstractThunk) = return Adjoint_mat_pullback(unthunk(ȳ)) | |||
| Adjoint_mat_pullback(ȳ::AbstractZero) = (NoTangent(), ȳ) | |||
There was a problem hiding this comment.
Just an FYI: we are considering handling this at the AD level, see: #442 (comment)
But that is currently blocked by ProjectTo PR which I am working on at the moment.
There was a problem hiding this comment.
Should these lines be removed before this is merged? Or commented # remove me once X?
There was a problem hiding this comment.
Ideally removed, but it needs Diffractor to automatically propagate AbstractZeros, something like Zygote's https://github.com/FluxML/Zygote.jl/blob/5b803d12f2d235755ba6f23a9017fc29fefd9985/src/compiler/chainrules.jl#L143
If Diffractor does not do that, we should make an issue about it (and link it here)
There was a problem hiding this comment.
So is it fine to leave this in or should I just remove that part for now?
There was a problem hiding this comment.
Yeah let's keep them in to make sure it all works for Diffractor and just link this: JuliaDiff/Diffractor.jl#25.
We can revisit when the issue above is solved
simeonschaub
force-pushed
the
kf/diffractorbackport
branch
from
09f3230
to
d95337d
Compare
simeonschaub
added
the
needs version bump
Codecov Report
@@ Coverage Diff @@
## master #463 +/- ##
==========================================
- Coverage 98.38% 98.04% -0.34%
==========================================
Files 21 22 +1
Lines 2287 2306 +19
==========================================
+ Hits 2250 2261 +11
- Misses 37 45 +8
Continue to review full report at Codecov.
|
src/rulesets/Base/nondiff.jl
Outdated
| @non_differentiable similar(::Any...) | ||
| @non_differentiable eltype(::Type) | ||
|
|
||
| @non_differentiable one(::Any) | ||
| @non_differentiable ones(::Any...) | ||
| @non_differentiable zero(::Any) | ||
| @non_differentiable zeros(::Any...) |
There was a problem hiding this comment.
| @non_differentiable similar(::Any...) | |
| @non_differentiable eltype(::Type) | |
| @non_differentiable one(::Any) | |
| @non_differentiable ones(::Any...) | |
| @non_differentiable zero(::Any) | |
| @non_differentiable zeros(::Any...) | |
| @non_differentiable eltype(::Type) |
could we also move the eltype to the right place alphabetically?
There was a problem hiding this comment.
Do you mean
| @non_differentiable similar(::Any...) | |
| @non_differentiable eltype(::Type) | |
| @non_differentiable one(::Any) | |
| @non_differentiable ones(::Any...) | |
| @non_differentiable zero(::Any) | |
| @non_differentiable zeros(::Any...) | |
| @non_differentiable eltype(::Type) | |
| @non_differentiable similar(::Any...) | |
| @non_differentiable one(::Any) | |
| @non_differentiable ones(::Any...) | |
| @non_differentiable zero(::Any) | |
| @non_differentiable zeros(::Any...) |
?
There was a problem hiding this comment.
No, sorry, I did mean to delete the others, since they are already in base.jl on master, see e.g.
ChainRules.jl/src/rulesets/Base/nondiff.jl
Line 307 in 3f18cd6
julia> @which ones
BaseThere was a problem hiding this comment.
actually eltype also seems to belong to Base
There was a problem hiding this comment.
Ah, sorry, I misunderstood. eltype should still be in Base/nondiff.jl though, right?
There was a problem hiding this comment.
Correct! My bad, forgot that Base/base.jl exists 😄
Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
simeonschaub
removed
the
needs version bump
No description provided.