-
Notifications
You must be signed in to change notification settings - Fork 62
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
Introduce MutableTangent #626
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
==========================================
+ Coverage 93.91% 94.15% +0.24%
==========================================
Files 15 15
Lines 904 976 +72
==========================================
+ Hits 849 919 +70
- Misses 55 57 +2 ☔ 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.
This is a pretty big change, but It looks good.
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 is cool. Glad to see it added. I've a few things to say about zero_tangent
because I'm implemented something similar recently.
src/tangent_types/abstract_zero.jl
Outdated
return Array{guess_zero_tangent_type(T),N} | ||
end | ||
guess_zero_tangent_type(::Any) = Any # if we had a general way to handle determining tangent type # https://github.com/JuliaDiff/ChainRulesCore.jl/issues/634 | ||
# TODO: we might be able to do better than this. even without. |
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.
[JuliaFormatter] reported by reviewdog 🐶
# TODO: we might be able to do better than this. even without. | |
# TODO: we might be able to do better than this. even without. |
It might make sense to test that this works for self-referential types c.f. something like this example from the docs. Maybe add a differentiable field of some kind? I'm not entirely sure how this will work -- I'm still trying to figure out how to avoid an infinite-loop in some of my work. |
I don't see any mention of self-referential types in the Swift docs on automatic tangent type synthesis, but I imagine they must've run into this so perhaps the implementation could have some clues? |
75c2ca3
to
7718bc7
Compare
Ok this has now been massively overhauled. However, in the process some things may have been broken. I expect MutableTangent will be missing some of the nicer overloads for linear operators etc., |
This property, in general, holds for all valid tangent types. |
This looks reasonable to me. |
The error in the projection tests is unrelated to this PR and is fixed in |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
f9ade6e
to
95e63d0
Compare
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 is cool.
Will close #105