EnzymeAD
[EnzymeTestUtils] Vectorize function for FiniteDifferencesCalls
This PR adds our own version of FiniteDifferences.to_vec, which allows EnzymeTestUtils.jl to correctly test rules with structured array inputs/outputs.
As noted in a few issues, e.g. https://github.com/EnzymeAD/Enzyme.jl/pull/1264#issuecomment-1921654762, FiniteDifferences (like ChainRules) represents tangents of structured arrays with arrays whose elements are semantically interpreted as tangent to the original array in its subspace of the dense arrays. Enzyme, on the other hand, treats arrays no differently from other structs. This difference is highlighted in how the two packages treat tangents of Symmetric. Both use a Symmetric container, but FD interprets the densified version of the tangent as tangent to the densified version of the original array, while Enzyme interprets the underlying .data of the tangent as tangent to the underlying .data of the original array.
The to_vec implementation included here allows us to convert all functions to the signature f(::DenseVector{<:AbstractFloat})::DenseVector{<:AbstractFloat} before calling FiniteDifferences, so that we control how the tangents are constructed.
This still needs tests for to_vec. It seems that on main the reverse-mode mutated callable test in EnzymeTestUtils has been erroring since #1282. The error changed in #1292.
@wsmoses e6fa10b marked some formerly broken EnzymeTestUtils tests as passing, but they were still failing when that PR was merged. Should that commit be reverted?
@wsmoses this PR is stalled pending answers to the questions in https://github.com/EnzymeAD/Enzyme.jl/pull/1327#discussion_r1516000282 and https://github.com/EnzymeAD/Enzyme.jl/pull/1327#issuecomment-1979737864
@wsmoses e6fa10b marked some formerly broken EnzymeTestUtils tests as passing, but they were still failing when that PR was merged. Should that commit be reverted?
Yeah this should be fixed here (that was itself intending to fix things since the default Enzyme BLAS internals had changed).
@wsmoses e6fa10b marked some formerly broken EnzymeTestUtils tests as passing, but they were still failing when that PR was merged. Should that commit be reverted?
Yeah this should be fixed here (that was itself intending to fix things since the default Enzyme BLAS internals had changed).
I disagree this should be fixed here, as it's unrelated to this PR and was already broken when #1282 was merged. If you have a fix you can add, that would be great, but I don't intend to work on that.
Got it. Since it currently prevents us from testing the changes here, I'll pick this up again when the EnzymeTestUtils CI passes on master.
@sethaxen If you don't have cycles, then mind making an issue then with the needs-help tag? It should just be setting the EnzymeTestUtils failures/skips to match what happens in CI.
@sethaxen enzymetestutils passes main now, if you have cycles to get this over the finish line
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 74.94%. Comparing base (
724b9bc) to head (b8ba210). Report is 1 commits behind head on main.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## main #1327 +/- ##
==========================================
- Coverage 75.40% 74.94% -0.47%
==========================================
Files 36 28 -8
Lines 10671 10407 -264
==========================================
- Hits 8047 7800 -247
+ Misses 2624 2607 -17
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Don't merge yet, need to resolve a bug on v1.7 first.
Found it, it's the example in #1394
Awesome, though unfortunately the solution to that is to use a later Julia as it is a bug in Julia's GC that was fixed. We should just version gate the test.
@wsmoses I added some more tests and tweaked some docstrings. I also needed to fix the random seed to get tests to pass. This is now ready to merge and could be merged before or after #1398.
LGTM lmk if you want me to merge (but I think you should have perms to)
On Sat, Apr 20, 2024 at 6:43 PM Seth Axen @.***> wrote:
@wsmoses https://github.com/wsmoses I added some more tests and tweaked some docstrings. I also needed to fix the random seed to get tests to pass. This is now ready to merge and could be merged before or after #1398 https://github.com/EnzymeAD/Enzyme.jl/pull/1398.
— Reply to this email directly, view it on GitHub https://github.com/EnzymeAD/Enzyme.jl/pull/1327#issuecomment-2067806047, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJTUXHRKQH3NEIBYJJNVLDY6LVRXAVCNFSM6AAAAABEHOG2KGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRXHAYDMMBUG4 . You are receiving this because you were mentioned.Message ID: @.***>