Update of #36 and #35 #93
Conversation
| AD.@primitive function value_and_pullback_function(ba::AD.ReverseRuleConfigBackend, f, xs...) | ||
| value, back = ChainRulesCore.rrule_via_ad(AD.ruleconfig(ba), f, xs...) | ||
| function value_and_pullback(vs) | ||
| pb_value = if vs === nothing |
There was a problem hiding this comment.
We have to handle nothing due to the way jacobian is implemented for @primitive function value_and_pullback_function (this PR) and @primitive function pullback_function (on the master branch):
There was a problem hiding this comment.
On the master branch, the default fallback for value_and_pullback_function handles nothing inputs, hence the design choice becomes apparent only in this PR.
There was a problem hiding this comment.
I'm not sure why the jacobian is implemented in this way. In principle, calling pullback repeatedly should be sufficient and the value should not be needed.
There was a problem hiding this comment.
Could be solved easily with #35 I assume since then the value could be obtained before constructing the co-tangents.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #93 +/- ##
==========================================
- Coverage 85.12% 84.12% -1.01%
==========================================
Files 8 8
Lines 464 466 +2
==========================================
- Hits 395 392 -3
- Misses 69 74 +5
☔ View full report in Codecov by Sentry. |
|
I solved #93 (comment) by merging and updating #35. With this change, |
|
@devmotion would it make sense to integrate #100 into the current PR? |
|
Then would you mind maybe reviewing it since it's rather short, and advising me on how to add tests? Since you've already worked on that part of the code, your opinion would be very valuable! |
|
Bump 🙂 #95 could be included in a breaking release as well. And of course, I'd be fine with merging some of the other non-breaking PRs first if they are reviewed and approved 🙂 |
|
@oxinabox ok for merging this breaking PR? |
|
Please do |
|
I thought part of ColPrac is that developers merge their own PRs after approval? 🤔😛 |
|
I was planning to read it this morning! And actually your PRs had been waiting for so long I thought you didn't have merge rights, and I didn't even check 😅 sorry about that |
|
Speaking of, does the repo have the right branch protections to help enforce ColPrac? |
|
Well, they weren't approved (and I also wanted to wait for Mohammeds approval specifically). I merged and released #97 as soon as it was approved yesterday 🙂 |
|
I apologize for the confusion. I'm now up to speed on ColPrac ;) |
This PR is an update of #36, without merge conflicts and with updated primitives of Tracker and CRC.
A concrete example of why #36 and this PR are useful is #57: This issue is fixed if the primal is taken directly from
rruleinstead of manually re-computing the primal.Fixes #57.
cc @sethaxen
Edit: I also updated and merged #35 in this PR to fix #93 (comment).
Fixes #34.