A[c|t]_mul_B[!] specializations for SparseMatrixCSC-StridedVecOrMat, less generalized linear indexing and meta-fu, take 2 #20053
Conversation
| A_mul_B(A::SparseMatrixCSC, B::StridedVecOrMat) = A_mul_B!(_argstuple_AqmulB!(A, B)...) | ||
| At_mul_B(A::SparseMatrixCSC, B::StridedVecOrMat) = At_mul_B!(_argstuple_AqmulB!(A, B)...) | ||
| Ac_mul_B(A::SparseMatrixCSC, B::StridedVecOrMat) = Ac_mul_B!(_argstuple_AqmulB!(A, B)...) | ||
| _argstuple_AqmulB!{TvA,TB}(A::SparseMatrixCSC{TvA}, b::StridedVector{TB}) = |
There was a problem hiding this comment.
these don't mutate their inputs, do they?
There was a problem hiding this comment.
Correct. I intended the name to convey that _ argstuple_AqmulB! generates the argument tuple for _AqmulB!, but perhaps something else would be better? Thoughts? Thanks!
There was a problem hiding this comment.
since they only get called on the rhs of definitions for non-mutating A?_mul_B I think leaving off the ! would still be clear
There was a problem hiding this comment.
Cheers, dropped the trailing !. Thanks!
| _argstuple_AqmulB!{TvA,TB}(A::SparseMatrixCSC{TvA}, B::StridedMatrix{TB}) = | ||
| (R = promote_type(TvA, TB); (one(R), A, B, zero(R), similar(B, R, (A.n, size(B,2))))) | ||
|
|
||
| A_mul_B!(α::Number, A::SparseMatrixCSC, B::StridedVecOrMat, β::Number, C::StridedVecOrMat) = |
There was a problem hiding this comment.
we should really deprecate this argument order, it's not called gemm so there's no reason for it to follow fortran conventions instead of julia ones
There was a problem hiding this comment.
Agreed. Referencing #16772 as a bread crumb to this case. Thanks!
There was a problem hiding this comment.
related but not quite identical
| _AqmulB_specialscale!(C::StridedVecOrMat, β::Number) = | ||
| β == 1 || (β == 0 ? fill!(C, zero(eltype(C))) : scale!(C, β)) | ||
|
|
||
| function _AqmulB_kernel!(α::Number, A::SparseMatrixCSC, ::typeof(identity), B::StridedVector, C::StridedVector) |
There was a problem hiding this comment.
likewise, julian argument order would have the output array first
There was a problem hiding this comment.
I think this should be discussed elsewhere. It is not really relevant to this PR.
There was a problem hiding this comment.
we're adding new functions here, they should make sense
There was a problem hiding this comment.
Agreed (edit: with op). When changing the user-facing methods' argument order, changing the kernel's argument order to match would be great.
There was a problem hiding this comment.
To clarify, I agree that changing the argument order of these five-argument A_mul_B! methods would be an improvement. And the corresponding kernels' argument order should match, so when the former changes the latter should also change. That argument reordering being orthogonal to this refactoring, however, I advocate we reorder the arguments in a separate pull request (or at least a separate commit); if reordering the arguments is a priority, I would be happy to see it through near-term. Thoughts? Thanks!
There was a problem hiding this comment.
The kernel has a different set of inputs than the outer function so I'm less attached to the kernel needing to have the same argument order. But sure, this can wait - it should be refactored when a matrix transpose type allows cleaning up all these names, we can also fix the argument order and drop the Fortran conventions once dispatch is doing most of the work. I am a bit curious how widely used the gemm style argument orders are for these functions (and why they aren't being called with the gemm name in those cases). If the cost of keyword arguments gets solved soon, it would probably make more sense to do the scalar multiples that way, maybe with more obvious names even. This is post-0.6 brainstorming though. Given mbauman's reworking of the generalized linear indexing to possibly not deprecate indexing with trailing 1's yet, would these PR's still be relevant?
…cOrMat combinations, without generalized linear indexing and meta-fu.
This all sounds fantastic!
I have been wondering the same. I see two reasons to see these PRs through: (1) If indexing with trailing singleton dimensions will be deprecated (at some point, if not immediately), these PRs will make that process smoother. (2) This PR makes these methods statically analyzable, which seems worthwhile in its own right. Thoughts? Thanks again! |
In what way? |
@vtjnash, from your blog post I took away that any use of eval and/or generated functions defeats static analysis/compilation. I imagine that understanding is simplistic? Could you clarify? Thanks! |
|
that is true, but we don't care if the compiler can evaluate that the body of the for-loop will cause the evaluation of new method definitions. |
|
Closing as no longer relevant given the evolution of indexing. @tkelman, is the argument order issue tracked elsewh ere? Or should we open a dedicated tracking issue? Thanks! |
|
I can't recall there being a separate issue talking about the outlier matmul methods with blas-like arguments and order, and what to do about them |
(This pull request is #20043, but instead targeting master rather than
mb/deprecate-17440per discussion elsewhere). Towards fixing some generalized linear indexing deprecation warnings in #20040, this pull request rewrites theA[c|t]_mul_B[!]specializations forSparseMatrixCSC-StridedVecOrMatcombinations, leveraging multiple dispatch and higher order functions to remove generalized linear indexing and meta-fu. Best!