-
Notifications
You must be signed in to change notification settings - Fork 56
Initial support for CUDA + factorizations #336
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
Conversation
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
633ceb2 to
c75dc4f
Compare
test/cuda/factorizations.jl
Outdated
| @test w * p ≈ t | ||
| @test isisometric(w) | ||
| @test isposdef(p) | ||
| @test isposdef(project_hermitian!(p)) |
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.
Do we need the projection here, and if so, why? Shouldn't the result already be hermitian?
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.
Currently in the extension we're checking the input is approximately hermitian then wrapping it in a Hermitian it if so for isposdef. I'll try removing the projection here (or maybe we need to project out the extraneous non posdef terms?).
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.
But positive definite requires hermitian anyways right? Testing if the result is positive definite should also mean testing it is hermitian, which is why I'm confused we have to project it to be hermitian, rather than checking for that property.
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.
I would also think that our left_polar implementations output a p factor which is exactly hermitian. If this is not the case, this is something to add to the TODO list of MAK.
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.
I think there is also now an actual problem with the test for AdjointTensorMap (related to copying?) which I'll dig into further today
lkdvos
left a comment
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.
Overall definitely looks good to me!
I'm slightly worried about the code duplication for the tests, similar to how we have that for MatrixAlgebraKit, but I also don't think there are better alternatives so I'm ok with merging like this, except for some small comments.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| function LinearAlgebra.isposdef(t::CuTensorMap) | ||
| domain(t) == codomain(t) || | ||
| throw(SpaceMismatch("`isposdef` requires domain and codomain to be the same")) | ||
| InnerProductStyle(spacetype(t)) === EuclideanInnerProduct() || return false | ||
| for (c, b) in blocks(t) | ||
| # do our own hermitian check | ||
| isherm = MatrixAlgebraKit.ishermitian(b; atol = eps(real(eltype(b))), rtol = eps(real(eltype(b)))) | ||
| isherm || return false | ||
| isposdef(Hermitian(b)) || return false | ||
| isposdef(project_hermitian!(b)) || return false | ||
| end | ||
| return true | ||
| end |
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.
Still confused by this. The CPU version doesn't have this approximate hermiticity check and then hermitian projection. It just fails directly if it is not exactly hermitian. Why does the "generic" implementation in factorizations.jl fail for CUDA? Does it not support isposdef!?
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.
isposdef! works BUT I think CUSOLVER is very brittle about what counts as being posdef (see https://github.com/JuliaGPU/CUDA.jl/blob/0c00b83fe9df007efcb3e738fa5f1973603f79c4/lib/cusolver/dense.jl#L905)
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.
So for example if you have an eigenvalue -1e-17 that may return "not positive definite"
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.
I think that we should really try and follow the CPU conventions for this, so indeed maybe this should just really check for exact hermitian, and go from there.
If this gives issues for the GPU projections, we might want to alter these implementations to put a project_hermitian at the end of these, rather than try and alter the checks?
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.
OK, so don't project_hermitian here, and make the check above exact?
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 exact ishermitian check is already part of isposdef; I think if the MAK implementation of PolarViaSVD is fixed (see Zulip), this standard isposdef check should work without need for a CUDA specialisation.
92cd3c2 to
623edf5
Compare
|
I think this would now be passing as soon as we tag a new version of MatrixAlgebraKit, due to the changes in QuantumKitHub/MatrixAlgebraKit.jl#143. |
6a2d615 to
56fd179
Compare
56fd179 to
fc83d58
Compare
Co-authored-by: Lukas Devos <[email protected]>
Co-authored-by: Jutho <[email protected]>
fc83d58 to
1702854
Compare
|
OK, CUDA on 1.10 is working. CUDA on 1.12 will be fixed by JuliaGPU/GPUArrays.jl#676. |
lkdvos
left a comment
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.
I would be happy to force merge this since these errors are unrelated, and just deal with the failing buildkite tests in the meantime.
In any case we'll have to tag a release of TensorKit to make sure the latest releases version has working svd and eigenvalue matrixalgebrakit implementations for diagonal
|
IMO let's push it and unblock the truncation stuff etc |
OK, now only truncation is not working, which is expected because it is also a WIP in MatrixAlgebraKit!