Avoid some unintended calls to generic_matmatmul! for special matrices #16615
Conversation
|
Out of curiosity, have you experimented anything with splitting up |
|
|
||
| function A_mul_Bt(D::Diagonal, A::AbstractMatrix) | ||
| Ac = similar(A, promote_op(*, eltype(A), eltype(D.diag)), (size(A, 2), size(A, 1))) | ||
| ctranspose!(Ac, A) |
|
@KristofferC I've tried splitting up |
|
|
||
| A_mul_B!(A::Diagonal,B::Diagonal) = throw(ArgumentError("mutation argument is ambiguous. Please promote one of the arguments.")) | ||
| At_mul_B!(A::Diagonal,B::Diagonal)= throw(ArgumentError("mutation argument is ambiguous. Please promote one of the arguments.")) | ||
| Ac_mul_B!(A::Diagonal,B::Diagonal)= throw(ArgumentError("mutation argument is ambiguous. Please promote one of the arguments.")) |
There was a problem hiding this comment.
This isn't really promotion in the type sense. The "ambiguous" error is a little strange. Do we try to follow mutating the first or second argument remotely consistently elsewhere?
There was a problem hiding this comment.
No. Following the BLAS tradition for triangular matrices, the non-diagonal matrix is mutated. These methods are only defined to avoid ambiguity warnings and it's unlikely that they will get called. The fact that these definitions are necessary makes we wonder if we should really run the ambiguity tests.
There was a problem hiding this comment.
This isn't really promotion in the type sense
Do you mean element type?
There was a problem hiding this comment.
I'm sorry, but the BLAS tradition is designed for Fortran. We can do better, we have dispatch. It only makes sense to do generic code if we try to be consistent about what verbs mean. You can do in-place multiplication of diagonal matrices, it's not an error. But it is confusing and misleading to have A_mul_B! sometimes mutate the first argument and sometimes the second depending what types they are. We should fix that and be consistent - what benefit is there to having this vary so widely?
There are no element types in this signature. You seem to be referring to a "promotion" in the structural sense, but I'd say calling full or equivalent is losing structural type information so making things worse, not really analogous to the scalar type promotion that we typically use that terminology to mean.
There was a problem hiding this comment.
I'm fine with changing the error message. It's a byproduct of an evening of ambiguity warning fixing so I'm sure that it can be improved. However, it's beyond the scope of this PR to introduce new rules for ambiguous multiplication situations. It might have more consequences than you realize so let's discuss that at JuliaCon.
There was a problem hiding this comment.
Improving consistency here is beyond the scope of this PR, sure. But this operation isn't an error.
There was a problem hiding this comment.
I've changed the errors to MethodErrors which is what we have now. Then we can discuss how to adding new functionality later. The present solution is not generally inconsistent. We just apply a different set of rules than those you have in mind. I'll merge when lights are green unless there are other comments.
There was a problem hiding this comment.
Sometimes mutating the first input and sometimes the second is incredibly inconsistent. I'll open an issue, but if 2-argument A_mul_B! (or mul! if that happens) doesn't have a generically predictable meaning then it shouldn't exist.
There was a problem hiding this comment.
@tkelman, how can you rob every future Julia user of that happy feeling you get reading Netlib documentation? You're being a real spoil-sport here 😉.
LQ and Diagonal had the same issue as QR.