[WIP] D* AD algorithm - #1888
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1888 +/- ##
==========================================
- Coverage 26.12% 17.78% -8.35%
==========================================
Files 56 62 +6
Lines 5356 6496 +1140
==========================================
- Hits 1399 1155 -244
- Misses 3957 5341 +1384 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
LordOfFrogs
changed the title
AayushSabharwal
left a comment
AayushSabharwal
left a comment
There was a problem hiding this comment.
I think the progress looks amazing! My comments are mainly around possible performance pitfalls. You did mention on slack that you're aware of some such cases but I thought it prudent to make sure we're on the same page about some of these pieces.
| end | ||
|
|
||
| # evaluate the product of single linear path of edges (this represents applying the chain rule) | ||
| function evaluate_path(dg::DerivativeGraph{T}, edge::Edge{T}, goal::T) where T |
There was a problem hiding this comment.
Can we end up calling evaluate_path with the same edge and goal twice? Is this something that we can then cache or otherwise optimize?
There was a problem hiding this comment.
There is actually a section of the D* paper that covers a solution to this problem of common subproducts, which I am currently working on implementing. The current evaluate_path function was just to verify that R1->R1 derivatives were working correctly.
| vars::Vector{SymbolicT} # variable index -> variable symbolic expression | ||
| varset::Set{SymbolicT} # for fast checking if an expression is a variable | ||
| var_idx_to_postorder::Vector{T} | ||
| var_idx_to_postorder::IdDict{Int,T} |
There was a problem hiding this comment.
I'm curious - why a map over an array? And why IdDict?
There was a problem hiding this comment.
I switched from an array to a map because in cases where a root has no children or a variable has no parents, it makes more sense to omit it from the postorder numbering because of how the code is structured, and using a map proved the easiest way to do that (I tried using nothings in the array, but that generated a bunch of new problems, and trying to give them postorder numbers also led to problems).
IdDict was just me following the pattern for the existing maps, but I realize now that the only reason I had them as IdDict's in the first place was because I was basing the struct off of FastDifferentiation.jl, which uses a Node struct instead of integers. They have been changed to Dict's now, which should be more performant.
IdDict -> Dict, removing erroneous collect calls, only -> first
Subgraphs now only focus on a specific set of roots/vars (its dominance) so subgraphs dont have to have dom/pdom relationships for every root+var; Includes many other more minor fixes with it that cropped up
subgraph edge exploration termination cases reordered add_edge! protection from duplicate edges corrected is_dominator/is_postdominator edge filtering
3 participants
Implements the D* AD algorithm based on this paper. Similar to FastDifferentiation.jl. Still a work-in-progress.
To be fixed/implemented:
DerivativeGraphrepresentation of expressionsifelse@register_derivativeBased on some initial benchmarking, this alorithm appears to be at least 2x faster than the existing one for relatively small expressions. In theory, this should improve even more for larger expressions and Rn-Rn functions.(after implementing fixes for large graphs, performance needs more work)