Skip to content

Conversation

@dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Apr 14, 2023

Fixes part of JuliaGPU/CUDA.jl#1866, resolves #46196 (comment). I removed some not exported functions that hadn't existed before #46196 anyway and added a few tests.

@dkarrasch dkarrasch added linear algebra Linear algebra bugfix This change fixes an existing bug labels Apr 14, 2023
@maleadt
Copy link
Member

maleadt commented Apr 15, 2023

Thanks!

@dkarrasch
Copy link
Member Author

Have you checked, does this fix the entire GPU package stack? Or will it be easier to merge this as is and then see what happens with nightly?

@dkarrasch
Copy link
Member Author

Let's go with this and then fix issues as/if they arise.

@dkarrasch dkarrasch merged commit 78fd05a into master Apr 17, 2023
@dkarrasch dkarrasch deleted the dk/abstractq_audit branch April 17, 2023 09:06
@maleadt
Copy link
Member

maleadt commented Apr 17, 2023

Sorry, didn't have time to test this over the weekend.
It fixes the immediate issue, but it looks like #46196 has broken other parts of CUDA.jl. For example, our tests were relying on collect(::QRCompactWYQ) to get an Array, but that doesn't seem to work anymore post #46196. I assume that's intended, as collect is documented to get an Array from anything iterable?

@dkarrasch
Copy link
Member Author

I see. I've "fixed" quite a couple of packages with this usage. If you're fine with the result being a plain Matrix, then replace collect by Matrix, or replace collect(Q) by Q*I (i.e., the UniformScaling I). That might be able to return a CuArray, if that's what the corresponding Q * true does. I hope this is just a test pattern, not a productive code pattern.

I don't think I can develop CUDA.jl locally, so should I just fork the repo, start a PR and test with online CI until it's all green?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This change fixes an existing bug linear algebra Linear algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants