feat: Add reverse/forward counterparts#1036
Conversation
rsenne
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1036 +/- ##
==========================================
- Coverage 98.21% 93.92% -4.29%
==========================================
Files 138 115 -23
Lines 8131 5947 -2184
==========================================
- Hits 7986 5586 -2400
- Misses 145 361 +216
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gdalle
left a comment
gdalle
left a comment
There was a problem hiding this comment.
Thank you for getting this started!
Most test failures are a red herring (test CI runs by forcing the highest possible version of each package, which here leads to incompatibilities between the recent OrderedCollections v2.0 and the not-yet-updated DataFrames)
|
I think I addressed everything:
Let me know if anything needs further adjustment! |
| # Backend-specific counterparts (e.g., for AutoMooncake and AutoEnzyme) are defined in the | ||
| # corresponding package extensions. |
There was a problem hiding this comment.
There are a few more counterparts we need to implement and test in DI core:
ADTypes.NoAutoDiff=> handled correctly by the defaults aboveADTypes.AutoSparse=> grabdense_backend, apply counterpart and rewrap?DI.SecondOrder=> no idea, cause applying the counterpart to bothinnerandouterdoesn't mean much. Maybe best to error hereDI.MixedMode=> do nothingDI.AutoForwardFromPrimitive=>DI.AutoReverseFromPrimitive(and apply counterpart inside? not sure)DI.AutoReverseFromPrimitive=>DI.AutoForwardFromPrimitive(and apply counterpart inside? not sure)
There was a problem hiding this comment.
I basically went with your judgement on everything here as I don;t know the functionality enough to have strong opinions. That said -- your suggestions all made sense to me FWIW
|
okay think i got everything again |
gdalle
left a comment
gdalle
left a comment
There was a problem hiding this comment.
LGTM except
- the warnings to remove
- the test failures for Zygote and ReverseDiff
- the missing coverage
| """ | ||
| function forward_counterpart(backend::AbstractADType) | ||
| if !(mode(backend) isa Union{ForwardMode, ForwardOrReverseMode, SymbolicMode}) | ||
| @warn "The forward-mode counterpart of `$backend` is itself, returning it unchanged." maxlog = |
There was a problem hiding this comment.
You're right, warnings will get annoying quickly, let's get rid of them
| """ | ||
| function reverse_counterpart(backend::AbstractADType) | ||
| if !(mode(backend) isa Union{ReverseMode, ForwardOrReverseMode, SymbolicMode}) | ||
| @warn "The reverse-mode counterpart of `$backend` is itself, returning it unchanged." maxlog = |
| test_counterparts(AutoZeroForward()) | ||
| test_counterparts(AutoZeroReverse()) |
There was a problem hiding this comment.
Why doesn't this show up in code coverage?
2 participants
PR to add the forward/backward counterparts discussed. I wasn't sure if for a generic backend if there was no reasonable counterpart to return e.g., if one did
reverse_counterpart(AutoForwardDiff)if the code should throw an ArgumentError or if it should just return the backend. I opted for the formerResolves #1025.