-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Replace Differential implementation with simplified Dual (2/N) #163
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #163 +/- ##
==========================================
- Coverage 87.75% 87.68% -0.07%
==========================================
Files 100 99 -1
Lines 15982 15811 -171
Branches 856 852 -4
==========================================
- Hits 14025 13864 -161
+ Misses 1101 1095 -6
+ Partials 856 852 -4 ☔ View full report in Codecov by Sentry. |
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.
Wow. This is absolutely phenomenal.
@@ -530,8 +531,11 @@ | |||
(letfn [(process-term [term] | |||
(g/simplify | |||
(s/mapr (fn rec [x] | |||
(if (d/differential? x) | |||
(d/map-coefficients rec x) | |||
(if (d/dual? x) |
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.
does this come up often? it looks like a kind of fmap
over Duals
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.
This has always seemed like a hack to me... but maybe not. yes, it's like an fmap
for sure, and it's what we do for simplify
. We'll have a similar line to add for tapes. What do you think? Should we add something more general?
*active-tags*) | ||
(apply max tags))) | ||
|
||
(defn tag+perturbation |
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.
could you just return the dual number? it has the tag inside it already?
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.
YES that is true... I added this on my scratch branch when I still had Differential
and it was a touch more expensive to get the tag out, so I wanted to save the effort.
Can we look at this together when I massage this to be compatible with tapes too?
(defmethod g/one-like [::dual] [_] 1) | ||
(defmethod g/identity-like [::dual] [_] 1) | ||
(defmethod g/freeze [::dual] [d] | ||
`[~'Dual |
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.
I swore a saw a dict-like implementation of this somewhere
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.
I had a pprint
implementation for tapes that did that:
https://github.com/mentat-collective/emmy/blob/main/src/emmy/tape.cljc#L285-L296
but I didn't use it for freeze
. Honestly I don't think these should ever make it out into a situation where we'd inspect the results of a freeze...
This PR
Differential
generalized dual and its term list algebra with a simplifiedDual
number typeemmy.util.vector-set
and tests, as these are no longer usednil
implementation forextract-tangent
, meaning thatnil
-valued functions now work withD
This new approach works because the
emmy.differential/*active-tags*
stack allows us to make sure that lifted binary operations always wrap their output in a newDual
with the tag assigned by the inner-most derivative call.