Skip to content

Conversation

@dkarrasch
Copy link
Contributor

This PR collects fixes to make CUDA.jl work properly with the new AbstractQ API.

@maleadt
Copy link
Member

maleadt commented Apr 17, 2023

Thanks! I would have been happy to make these changes, just wanted to make sure I understood the intention.
Local testing revealed there's some other issues lurking though. Let's see what CI thinks.

@maleadt maleadt added the cuda array Stuff about CuArray. label Apr 17, 2023
@dkarrasch
Copy link
Contributor Author

No worries. I'll just step through the issues as they arise. But feel free to speed up the process by pushing commits.

Yes, the intention was correct. The issue is that Qs are not stored in the matrix representation, so any iteration through the matrix coefficients requires to execute code (and potentially allocate memory etc.), so that was a poorly performing code pattern even before my Julia Base PR. And, in fact, part of the motivation was to reveal that code pattern by disallowing broadcasting and iteration on AbstractQ types.

@dkarrasch
Copy link
Contributor Author

I think to fix the test on line 395 requires a new conversion method.

convert(::Type{T}, ::[QRPackedQ/AbstractQ?]) where {T<:CuArray} = lmul!(Q, T(I, size(Q))

that is multiplication of Q into the desired CuArray type?

@dkarrasch
Copy link
Contributor Author

The challenge with the test is actually that there is only one path to materialize an AbstractQ, and a materialization is not readily available. So I wonder what we actually test when we compare two matrix representations. I think that would make more sense for a given, fixed matrix, and then we would compare with a fixed Q-matrix. I propose to compute Q'Q and compare to the identity matrix.

@maleadt
Copy link
Member

maleadt commented Apr 18, 2023

I'm not sure what the exact intent of those tests is, as I didn't write them. Feel free to restructure them, or even change the wrappers (or suggest how to), as ultimately the goal of this functionality is compatibility with LinearAlgebra but using GPU types.

@dkarrasch
Copy link
Contributor Author

JuliaLang/julia#49424 should fix the remaining conversion issue.

@dkarrasch
Copy link
Contributor Author

Now nightly is no longer failing, but timing out...

@maleadt
Copy link
Member

maleadt commented Apr 24, 2023

Couldn't reproduce the hang locally, and seems to work on CI now too, so let's merge this. Thanks for looking into the failures!

@maleadt maleadt merged commit a1437f5 into JuliaGPU:master Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda array Stuff about CuArray.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants