Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support nested forward/reverse mode #156
Support nested forward/reverse mode #156
Changes from 5 commits
f343c88
69ee50a
3ece359
deedf55
2617c36
1e86dc5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
building on the cryptic
fold-chain
from a previous PR, now we can drop this in and support reverse-mode.Check warning on line 300 in src/emmy/abstract/function.cljc
Codecov / codecov/patch
src/emmy/abstract/function.cljc#L300
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.
it is odd that nothing triggers L300, so in every case,
(and (tape/tape? entry) (= tag (tape/tape-tag entry)))
.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 would happen in the case of nested
gradient
calls to a literal function, I just don't have those in the tests yet. In that case, the innermost tag wins and any tag with a different tape is treated as a scalar (i.e. partial derivative == 0, so we never add it to the map)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 want to share tests between forward mode and reverse mode... when I do that this should get hit.
Check warning on line 322 in src/emmy/abstract/function.cljc
Codecov / codecov/patch
src/emmy/abstract/function.cljc#L322
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 feels janky, and is the only place where we deliberately introspect these types.
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.
looks like the inner thing is
mapv
too. Does writing it that way feel less janky?[node partial]
is almost an instance of Dual, right, except that the tag belongs to the containing tape. Armed with that, you could unify this part with the part above, but (I say sincerely) it would be too clever. Therefore I don't think this is janky: it feels like about what we want to see hereThere 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 whole function is too clever, and my cleverness bit me here: #168
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.
cleverness in trying to make this compatible with
D
, that is...