-
Notifications
You must be signed in to change notification settings - Fork 5
WIP: native algorithms #90
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
... and 29 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
I think having the native AD can be helpful for testing since the compilers in both Mooncake and Enzyme are pretty good and the guidance is to only write a custom rule if you really have to -- such as when you have a call to a foreign library like BLAS. |
|
This now contains a fully functional QR and LQ and tests, and could in principle be merged as is, with other factorizations be done in separate PRs. However, going forward, this raises a number of interesting questions: @lkdvos and @kshyatt
|
|
For 1, I think that since the arbitrary element types are the primary candidates for these native implementations, it would make sense to just register the native implementations as the defaults. I would prefer to avoid having to define overloads for every possible combination of weird element type and weird array type, and this seems like a solution that might often just work? For 2, I don't really know. We don't really have a good interface for selecting different AD modes right now, (nor for selecting tolerances etc), so while I don't mind having the option to use native AD, I would also just want to see what the performances are like before we invest into trying to make this accessible? |
test/lq.jl
Outdated
|
|
||
| eltypes = (Float32, Float64, ComplexF32, ComplexF64) | ||
| lapack_eltypes = (Float32, Float64, ComplexF32, ComplexF64) | ||
| native_eltypes = (lapack_eltypes..., BigFloat, Complex{BigFloat}) |
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.
Should we also try Float16 for the native eltypes?
|
Since this provides LQ and QR could we also test the |
|
Also, it looks like we are not yet testing the GPU support for these and I suspect we'll get scalar indexing errors -- should we add in GPU tests to this PR? |
|
This is definitely not meant for GPUs (which I still do not know anything about). Do people ever want to do non-native scalar types on GPUs? Adding So I then add a catchall Finally, regarding AD: I assume that our default setup will already automatically select our custom pullback rules also for these native QR and LQ implementations. How difficult is it to circumvent these registered custom pullback rules and switch to native AD'ing, e.g. for testing purposes? |
Well, Sander might ;). But people definitely do |
|
Since we are intercepting the AD at the level of |
This is currently just a proof of principle, but I think it is not too hard, using my old attempt at a generic linear algebra library (never published) to implement native algorithms. Currently only has a QR (which is easy), but with a performance that is surprisingly identical to GenericLinearAlgebra.
Hence, this is an alternative to #87 , but we can easily have both, as it would take some time to bring everything up to date.