JuliaDiff / AbstractDifferentiation.jl

An abstract interface for automatic differentiation.
https://juliadiff.org/AbstractDifferentiation.jl/
MIT License
135 stars 18 forks source link

Update of #36 and #35 #93

Closed devmotion closed 11 months ago

devmotion commented 1 year ago

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 rrule instead of manually re-computing the primal.

Fixes #57.

cc @sethaxen

Edit: I also updated and merged #35 in this PR to fix https://github.com/JuliaDiff/AbstractDifferentiation.jl/pull/93#discussion_r1279975890.

Fixes #34.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 86.95% and project coverage change: -1.01% :warning:

Comparison is base (00181f8) 85.12% compared to head (fe86f18) 84.12%.

Additional details and impacted files ```diff @@ 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 ``` | [Files Changed](https://app.codecov.io/gh/JuliaDiff/AbstractDifferentiation.jl/pull/93?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff) | Coverage Δ | | |---|---|---| | [src/AbstractDifferentiation.jl](https://app.codecov.io/gh/JuliaDiff/AbstractDifferentiation.jl/pull/93?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL0Fic3RyYWN0RGlmZmVyZW50aWF0aW9uLmps) | `78.46% <72.72%> (-2.05%)` | :arrow_down: | | [ext/AbstractDifferentiationChainRulesCoreExt.jl](https://app.codecov.io/gh/JuliaDiff/AbstractDifferentiation.jl/pull/93?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-ZXh0L0Fic3RyYWN0RGlmZmVyZW50aWF0aW9uQ2hhaW5SdWxlc0NvcmVFeHQuamw=) | `100.00% <100.00%> (ø)` | | | [ext/AbstractDifferentiationFiniteDifferencesExt.jl](https://app.codecov.io/gh/JuliaDiff/AbstractDifferentiation.jl/pull/93?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-ZXh0L0Fic3RyYWN0RGlmZmVyZW50aWF0aW9uRmluaXRlRGlmZmVyZW5jZXNFeHQuamw=) | `100.00% <100.00%> (ø)` | | | [ext/AbstractDifferentiationTrackerExt.jl](https://app.codecov.io/gh/JuliaDiff/AbstractDifferentiation.jl/pull/93?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-ZXh0L0Fic3RyYWN0RGlmZmVyZW50aWF0aW9uVHJhY2tlckV4dC5qbA==) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

devmotion commented 1 year ago

I solved https://github.com/JuliaDiff/AbstractDifferentiation.jl/pull/93#discussion_r1279975890 by merging and updating #35. With this change, nothing is not "abused" as co-tangent anymore in AbstractDifferentiation, the code could be simplified quite a bit, and implementations of value_and_pullback_function do not have to handle nothing anymore.

gdalle commented 1 year ago

@devmotion would it make sense to integrate #100 into the current PR? There I tried to handle the tuple and the single-input cases correctly (to resolve #99), but I'm not confident enough with the package to decide whether I did it right. Also it needs testing but the AbstractDifferentiation.jl test base is still scary to me ^^

devmotion commented 1 year ago

I guess it could but I'm hesitant right now because #100 still seems to be quite preliminary and #100 is supposed to be a bugfix that could be released in a non-breaking release whereas this PR is clearly breaking.

gdalle commented 1 year ago

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!

devmotion commented 1 year ago

Bump :slightly_smiling_face:

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 :slightly_smiling_face:

gdalle commented 12 months ago

@oxinabox ok for merging this breaking PR?

oxinabox commented 11 months ago

Please do

devmotion commented 11 months ago

I thought part of ColPrac is that developers merge their own PRs after approval? 🤔😛

gdalle commented 11 months ago

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

gdalle commented 11 months ago

Speaking of, does the repo have the right branch protections to help enforce ColPrac?

devmotion commented 11 months ago

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 🙂

gdalle commented 11 months ago

I apologize for the confusion. I'm now up to speed on ColPrac ;)