Avoid aliasing in in UniformScaling*AbstractMatrix #18286
Conversation
|
There was a long discussion in #15546 (comment) about whether return values should be dealiased or not. It seems important to settle that issue sooner than later, especially if the conclusion is in favor of aliasing. |
|
Thanks for the reference. I'd forgotten that discussion. However, as I read the discussion it covers several cases and the value dependent aliasing behavior was pretty close to be considered undesirable. It's also how |
|
Fair enough. |
| *(A::AbstractMatrix, J::UniformScaling) = J.λ == 1 ? A : J.λ*A | ||
| *(J::UniformScaling, A::AbstractVecOrMat) = J.λ == 1 ? A : J.λ*A | ||
| *(A::AbstractMatrix, J::UniformScaling) = J.λ == 1 ? copy(A) : J.λ*A | ||
| *(J::UniformScaling, A::AbstractVecOrMat) = J.λ == 1 ? copy(A) : J.λ*A |
There was a problem hiding this comment.
Neither the new definition nor the previous one is type-stable (eg. UniformScaling(1.0) * eye(Int, 3) returns a Matrix{Int}). Should it be fixed?
andreasnoack
force-pushed
the
anj/uniform
branch
from
762065d to
4e4423a
Compare
|
|
||
| function *(A::AbstractMatrix, J::UniformScaling) | ||
| if J.λ == 1 | ||
| T = promote_op(*, eltype(A), eltype(J)) |
There was a problem hiding this comment.
why have the special case at all?
...and remove unnecessary UniformScaling*SparseMatrixCSC methods
andreasnoack
force-pushed
the
anj/uniform
branch
from
4e4423a to
adbfb68
Compare
|
The special casing of 1 does seem out-of-place. Shouldn't it be done in I know we're trying to avoid having too many types, but if |
|
@cstjean I've removed the special casing. It might make sense to special-case it for performance, but then I think it should happen in the code for I don't think it is necessary to avoid the copy for performance by introducing a new type. Where performance is critical, the user will probably use |
|
@tkelman I've updated the tests to make sure that the non-commutative case is tested. I realized that |
* Avoid aliasing in in UniformScaling*AbstractMatrix ...and remove unnecessary UniformScaling*SparseMatrixCSC methods * Broaden the tests for non-commutative multiplication * Add Quaternion test case for q*[q] and clean up the Quaternion test type
* Avoid aliasing in in UniformScaling*AbstractMatrix ...and remove unnecessary UniformScaling*SparseMatrixCSC methods * Broaden the tests for non-commutative multiplication * Add Quaternion test case for q*[q] and clean up the Quaternion test type (cherry picked from commit cd94c99)
* Avoid aliasing in in UniformScaling*AbstractMatrix ...and remove unnecessary UniformScaling*SparseMatrixCSC methods * Broaden the tests for non-commutative multiplication * Add Quaternion test case for q*[q] and clean up the Quaternion test type (cherry picked from commit cd94c99)
...and remove unnecessary
UniformScaling*SparseMatrixCSCmethods