Refactor ChainRulesCoreExt into separate files #133
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #133 +/- ##
==========================================
+ Coverage 82.19% 82.57% +0.37%
==========================================
Files 43 48 +5
Lines 5713 5710 -3
==========================================
+ Hits 4696 4715 +19
+ Misses 1017 995 -22 ☔ View full report in Codecov by Sentry. |
| return NoTangent(), NoTangent(), ∂t | ||
| end | ||
| return A, convert_pullback | ||
| end | ||
|
|
||
| function ChainRulesCore.rrule(::typeof(Base.convert), ::Type{Dict}, t::AbstractTensorMap) | ||
| out = convert(Dict, t) | ||
| function convert_pullback(c) | ||
| function convert_pullback(c′) | ||
| c = unthunk(c′) | ||
| if haskey(c, :data) # :data is the only thing for which this dual makes sense |
There was a problem hiding this comment.
How about dual = typeof(out)(:data => c[:data]) ?
There was a problem hiding this comment.
I don't think that works, because then the spaces are missing in the dictionary-to-tensormap converter. Maybe the comment is a bit misleading -- all fields in the dictionary are required, but Zygote tends to drop fields that do not contribute (i.e. codomain and domain). Thus, this uses the dictionary from the forwards pass with the data from the backwards pass.
There was a problem hiding this comment.
Ok. I wanted to avoid copying the data from out, but probably copy(out) is a shallow copy that does not duplicate out[:data]?
There was a problem hiding this comment.
Indeed, it should really only duplicate the pointer, which I think is acceptable. On top of that, I am not sure this rule is ever used/useful anyways. I think I copied this at some point from maarten, but presumably that was also only introduced for testing purposes, as it also suffers from the weird interplay of "inner product on the parameters" for non-abelian symmetries.
lkdvos
force-pushed
the
refactor-chainrules
branch
from
b0e8230 to
99f71c0
Compare
This refactors the chainrules extension into separate files.
In the meantime, I improved the test coverage a bit, and ironed out some type instabilities.