Fixed reducedim operations for CuQRPackedQ #1118
Conversation
|
I understand your motivation, but I don't like the fix 😛 What's special about |
maleadt
added
cuda array
|
Those four reduction operators are all dispatched in a similar manner: https://github.com/JuliaLang/julia/blob/bb2d8630f3aeb99c38c659a35ee9aa57bb71012a/base/reducedim.jl#L885 So I thought to handle them all equally. Feel free to close this if it's not needed. |
Codecov Report
@@ Coverage Diff @@
## master #1118 +/- ##
==========================================
- Coverage 79.44% 79.37% -0.07%
==========================================
Files 118 118
Lines 8001 8006 +5
==========================================
- Hits 6356 6355 -1
- Misses 1645 1651 +6
Continue to review full report at Codecov.
|
| @@ -26,6 +26,14 @@ Base.size(A::CuQRPackedQ, dim::Integer) = 0 < dim ? (dim <= 2 ? size(A.factors, | |||
| CUDA.CuMatrix(A::CuQRPackedQ) = orgqr!(copy(A.factors), A.τ) | |||
| CUDA.CuArray(A::CuQRPackedQ) = CuMatrix(A) | |||
| Base.Matrix(A::CuQRPackedQ) = Matrix(CuMatrix(A)) | |||
| Base.similar(A::CuQRPackedQ{S,T}) where {S,T} = CuArray{S, ndims(a)}(undef, size(a)) | |||
There was a problem hiding this comment.
a vs A
T and S are switched around compared to the CuQRPackedQ definition
Also, can't you get N statically from S typevar?
| @@ -26,6 +26,14 @@ Base.size(A::CuQRPackedQ, dim::Integer) = 0 < dim ? (dim <= 2 ? size(A.factors, | |||
| CUDA.CuMatrix(A::CuQRPackedQ) = orgqr!(copy(A.factors), A.τ) | |||
| CUDA.CuArray(A::CuQRPackedQ) = CuMatrix(A) | |||
| Base.Matrix(A::CuQRPackedQ) = Matrix(CuMatrix(A)) | |||
| Base.similar(A::CuQRPackedQ{S,T}) where {S,T} = CuArray{S, ndims(a)}(undef, size(a)) | |||
|
|
|||
| # CuQRPackedQ isn't an AbstractGPUArray, so we miss out on lots of GPU-compatible methods. | |||
There was a problem hiding this comment.
Don't we want the same for CuQR?
There was a problem hiding this comment.
Shouldn't it be an AbstractGPUArray?
There was a problem hiding this comment.
How? It's currently a AbstractQ already, and presumable that's more useful.
I meant: why |
maleadt
added
needs changes
| @@ -26,6 +26,14 @@ Base.size(A::CuQRPackedQ, dim::Integer) = 0 < dim ? (dim <= 2 ? size(A.factors, | |||
| CUDA.CuMatrix(A::CuQRPackedQ) = orgqr!(copy(A.factors), A.τ) | |||
| CUDA.CuArray(A::CuQRPackedQ) = CuMatrix(A) | |||
| Base.Matrix(A::CuQRPackedQ) = Matrix(CuMatrix(A)) | |||
| Base.similar(A::CuQRPackedQ{S,T}) where {S,T} = CuArray{S, ndims(a)}(undef, size(a)) | |||
|
|
|||
| # CuQRPackedQ isn't an AbstractGPUArray, so we miss out on lots of GPU-compatible methods. | |||
There was a problem hiding this comment.
Shouldn't it be an AbstractGPUArray?
maleadt
force-pushed
the
master
branch
11 times, most recently
from
5ceeb9f to
a4a071b
Compare
maleadt
force-pushed
the
master
branch
4 times, most recently
from
5d585c4 to
c850163
Compare
Applying Base.sum/prod/maximum/minimum to CuQRPackedQ raises
ERROR: Scalar indexing is disallowedCasting to CuArray fixes the issue. Also, Base.similar returned a CPU matrix when applied to CuQRPackedQ @DhairyaLGandhi