-
Notifications
You must be signed in to change notification settings - Fork 17
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
Adapt to Enzyme v0.13 #471
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #471 +/- ##
==========================================
- Coverage 98.51% 97.83% -0.68%
==========================================
Files 107 106 -1
Lines 4298 4305 +7
==========================================
- Hits 4234 4212 -22
- Misses 64 93 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Yeah the testing is a bit chaotic because I have a separate GitHub Actions workflow and a separate set of dependencies for each AD backend. Otherwise it's very hard to instantiate an environment with all 12 of them, especially since some are nearly unmaintained. As a result, testing locally kinda sucks, but if you become more involved in DI development we may be able to figure out something smarter. |
I have not seen an explicit list of breaking changes, I just went through and tested all the basic cases myself. Seems to be almost done, but as you can see it's not quite there. Note that, as per our conversation on slack, this is going to have to flatten the Enzyme jacobians, which, as of writing, is not in yet. |
I now have this flattening the jacobians. Tests now seem to mostly pass, but there are still some cases where it's mysteriously returning all zeros. Btw, it's unfortunate that throwing test errors doesn't actually tell you what failed, it just shows you what the result is (because it's just showing the standard |
DI extensions (Enzyme)
DI source
DI tests
DIT tests
|
Thanks for finishing this. |
EDIT by @gdalle: see final description below
I'm a little bewildered by testing of this package, so I haven't checked this thoroughly yet and likely something somewhere will break, but it seems to mostly work.