-
Notifications
You must be signed in to change notification settings - Fork 2
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
Move Enzyme.jl into extension #71
Conversation
Benchmark Results
|
Very nice change, I like it! Just one thought: What do you think about using https://github.com/gdalle/DifferentiationInterface.jl directly? It's a smaller dependency compared to Enzyme and should give us a plug and play solution for all supported AutoDiff backends. |
I must admit I forgot there are packages for providing common autodiff interface, when I wrote this PR. Thank you for reminding! The issue that motivated this PR was that in my project I use both Turing.jl and FactorRotations.jl, so I run into some obscure version conflicts, which also affected unrelated packages. My suggestion is to keep this PR as is, and reconsider the support for DiffInterface.jl/ADTypes.jl if the need for more extensive autodiff support arises. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc guide implementing_rotation_methods
is still failing because it relies on autodiff for MyQuartimax
. Adding Enzyme as a dependency and call using Enzyme
should be sufficient to make it work again. Maybe we can add a note that the autodiff backend has to be explicitly loaded.
That's a fair assessment. I've added some minor change requests, otherwise this is good to go. Thanks for the effort! |
so that we can test for missing Enyzme behaviour
Enzyme.jl is quite a heavy dependency (and comes with its own compatibility constraints), but it is not essential for factor rotations, as it is only used by ComponentLoss criteria family.
This PR does the following:
autodiff_gradient!(::AutodiffBackend{:Enzyme}, ...)
implementation to EnzymeExtSo the only user-visible change after this PR would be to execute
using Enzyme
if autodiff is required.It also opens the possibility to have alternative backends for autodiff in the future.