Clarify which arg is modified in mul/div. #25421

simonbyrne · 2018-01-05T22:18:35Z

Another stab at #16772. This is more or less the same as #24698 except updated for Transpose.

JeffBezanson · 2018-01-05T22:42:35Z

I suppose this solution is ok. The numbers aren't beautiful, but neither is needing to repeat an argument (especially if the argument isn't just a variable name).

Sacha0

ldiv!(A, B) always overwrites B, and rdiv!(A, B) always overwrites A, so two-argument ldiv! and rdiv! do not share mul!'s relevant semantic ambiguity. Perhaps forgo attaching 2 to ldiv! and 1 to rdiv!? :)

simonbyrne · 2018-01-06T00:14:59Z

That's a good point. I renamed them for consistency, but would be happy to change them back.

Sacha0 · 2018-01-06T00:18:31Z

I renamed them for consistency, but would be happy to change them back.

Might as well minimize the aesthetic suboptimality :).

KristofferC · 2018-01-06T12:09:19Z

The convention is to modify the first argument. Therefore, for the function that adhere to the convention they can just be mul! and only those that break the convention needs a number.

Or is mul! an alias to mul!1 somewhere in this PR? Being forced to write mul!1 everywhere seems a bit unfortunate.

tkelman · 2018-01-06T13:19:37Z

There isn't an ambiguity in div, but ldiv does deviate from the argument order convention, and flagging that as such in the name seems beneficial to me. Could be done independently of changes to mul though.

simonbyrne · 2018-01-06T21:58:50Z

I'm not too fussed on the names, but as they're now unique it is simply a matter of sed to change them. If we want to rename the div ones as well, I have moved the changes their to the sb/div12 branch.

My main objection to mul1! being the 2-argument mul! is that mul2! is by far the more commonly used variant.

Sacha0 · 2018-01-07T04:39:03Z

base/deprecated.jl

-    @deprecate A_mul_Bc!(A::AbstractMatrix,B::Diagonal)     mul!(A, Adjoint(B))
+    function A_mul_B!(A::Diagonal,B::Diagonal)
+        depwarn("`A_mul_B!(A::Diagonal,B::Diagonal)` should be replaced with `mul1!(A, B)` or `mul2!(A, B)`.", :A_mul_B!)
+        throw(MethodError(A_mul_B!, Tuple{Diagonal,Diagonal}))


MethodError(A_mul_B!, (A, B))?

good catch.

Sacha0 · 2018-01-07T04:47:48Z

base/deprecated.jl

-    @deprecate Ac_mul_B!(A::LQPackedQ{T}, B::StridedVecOrMat{T}) where {T<:BlasComplex} mul!(Adjoint(A), B)
+    @deprecate A_mul_B!(A::LQ{T}, B::StridedVecOrMat{T}) where {T<:BlasFloat}   mul2!(A, B)
+    @deprecate A_mul_B!(A::LQ{T}, B::QR{T}) where {T<:BlasFloat}    A*B
+    @deprecate A_mul_B!(A::QR{T}, B::LQ{T}) where {T<:BlasFloat}    A*B


Why transform from mul!(A, B) to A*B? :)

Because they were implemented internally as copying already anyway

Perhaps we should nix the corresponding mul! methods, being something of misnomers? (Perhaps you already have? :) )

Sacha0 · 2018-01-07T04:50:50Z

base/deprecated.jl

+    @deprecate Ac_ldiv_B!(A::Union{LowerTriangular,UnitLowerTriangular}, B::UpperTriangular)    ldiv1!(Adjoint(A), B)
+    @deprecate Ac_ldiv_B!(A::Union{UpperTriangular,UnitUpperTriangular}, B::LowerTriangular)    ldiv1!(Adjoint(A), B)
+    @deprecate At_ldiv_B!(A::Union{LowerTriangular,UnitLowerTriangular}, B::UpperTriangular)    ldiv1!(Transpose(A), B)
+    @deprecate At_ldiv_B!(A::Union{UpperTriangular,UnitUpperTriangular}, B::LowerTriangular)    ldiv1!(Transpose(A), B)


good catch.

Sacha0 · 2018-01-07T04:52:02Z

base/deprecated.jl

@@ -3115,7 +3119,7 @@ end
    @deprecate At_mul_B!(C::AbstractVector  , A::AbstractTriangular, B::AbstractVector)     mul!(C, Transpose(A), B)
    @deprecate At_mul_B!(C::AbstractMatrix  , A::AbstractTriangular, B::AbstractVecOrMat)   mul!(C, Transpose(A), B)
    @deprecate At_mul_B!(C::AbstractVecOrMat, A::AbstractTriangular, B::AbstractVecOrMat)   mul!(C, Transpose(A), B)
-    @deprecate A_mul_B!(A::Tridiagonal, B::AbstractTriangular)  mul!(A, B)
+    @deprecate A_mul_B!(A::Tridiagonal, B::AbstractTriangular)  mul2!(A, B)


The odd method :).

Agreed it is odd. We could get rid of it?

Sacha0 · 2018-01-07T04:52:34Z

base/deprecated.jl

-        @deprecate A_mul_B!(A::$t{T,<:StridedMatrix}, B::StridedMatrix{T}) where {T<:BlasFloat}     mul!(A, B)
-        @deprecate A_mul_B!(A::StridedMatrix{T}, B::$t{T,<:StridedMatrix}) where {T<:BlasFloat}     mul!(A, B)
+        @deprecate A_mul_B!(A::$t{T,<:StridedMatrix}, B::StridedMatrix{T}) where {T<:BlasFloat}     mul2!(A, B)
+        @deprecate A_mul_B!(A::StridedMatrix{T}, B::$t{T,<:StridedMatrix}) where {T<:BlasFloat}     mul2!(A, B)


good catch.

Sacha0 · 2018-01-07T04:54:02Z

base/deprecated.jl

+    @deprecate A_mul_B!(G::Givens, R::Rotation)             mul2!(G, R)
+    @deprecate A_mul_Bc!(A::AbstractMatrix, G::Givens)      mul1!(A, Adjoint(G))
+    @deprecate A_mul_B!(G::Givens, A::AbstractVecOrMat)     mul2!(G, A)
+    @deprecate A_mul_B!(G1::Givens, G2::Givens)             G1 * G2


Why transform from mul!(G1, G2) to G1 * G2? :)

The method just threw an error anyway.

Sacha0 · 2018-01-07T04:54:31Z

base/deprecated.jl

-    @deprecate A_mul_B!(G1::Givens, G2::Givens)             mul!(G1, G2)
+    @deprecate A_mul_Bc!(A::AbstractMatrix, R::Rotation)    mul1!(A, Adjoint(R))
+    @deprecate A_mul_B!(R::Rotation, A::AbstractMatrix)     mul2!(R, A)
+    @deprecate A_mul_B!(G::Givens, R::Rotation)             mul2!(G, R)


Another tricky one :).

What's tricky about this?

Without knowing the implementations of Rotation and Givens, from this signature one might not know whether this operation is possible and, if so, what it should do. (This signature certainly would have given me pause had I not recently looked at the relevant definitions :).)

Yes, exactly.

Sacha0 · 2018-01-07T04:56:00Z

base/deprecated.jl

+
+@deprecate *(A::Factorization, B::Factorization) Matrix(A)*Matrix(B)
+@deprecate *(A::Adjoint{<:Any,<:Factorization}, B::Factorization) adjoint(Matrix(A.parent)) * Matrix(B)
+@deprecate *(A::Factorization, B::Adjoint{<:Any,<:Factorization}) Matrix(A) * adjoint(Matrix(B.parent))


Are these deprecations related?

Good question. I think I added them to deal with the QR/LQ business, but they may no longer be necessary.

I think the story was that there were a couple of these methods, but they were inconsistently defined and mostly just did this anyway. It is probably better to define more specific deprecations though.

Sacha0 · 2018-01-12T00:08:01Z

base/linalg/hessenberg.jl

 function (*)(Q::HessenbergQ{T}, X::StridedVecOrMat{S}) where {T,S}
    TT = typeof(zero(T)*zero(S) + zero(T)*zero(S))
-    return mul!(Q, copy_oftype(X, TT))
+


Why the additional empty line? :)

Sacha0 · 2018-01-12T00:09:45Z

base/linalg/linalg.jl

@@ -267,7 +278,7 @@ The reason for this is that factorization itself is both expensive and typically
 and performance-critical situations requiring `rdiv!` usually also require fine-grained
 control over the factorization of `B`.
 """
-rdiv!(Y, A, B)
+rdiv!(A, B)


Does a three-argument rdiv! docstring still exist somewhere?

There is no 3-arg rdiv!

Now that's amusing 😄.

Sacha0 · 2018-01-12T00:31:55Z

stdlib/SuiteSparse/src/SuiteSparse.jl

@@ -5,7 +5,7 @@ __precompile__(true)
 module SuiteSparse

 import Base: \
-import Base.LinAlg: ldiv!, rdiv!
+import Base.LinAlg: ldiv!, ldiv!


ldiv!, ldiv!?

A remnant from sed...

Sacha0 · 2018-01-12T00:32:13Z

stdlib/SuiteSparse/src/cholmod.jl

@@ -1,3 +1,4 @@
+


Sacha0 · 2018-01-12T00:32:57Z

stdlib/SuiteSparse/src/umfpack.jl

@@ -385,6 +385,8 @@ function nnz(lu::UmfpackLU)
 end

 ### Solve with Factorization
+# TODO: these should probably be removed since they're not inplace


Sacha0 · 2018-01-12T00:33:13Z

stdlib/SuiteSparse/src/umfpack.jl

@@ -398,6 +400,7 @@ ldiv!(translu::Transpose{Float64,<:UmfpackLU{Float64}}, B::StridedVecOrMat{<:Com
 ldiv!(adjlu::Adjoint{Float64,<:UmfpackLU{Float64}}, B::StridedVecOrMat{<:Complex}) =
    (lu = adjlu.parent; ldiv!(B, Adjoint(lu), copy(B)))

+


Sacha0 · 2018-01-12T00:35:22Z

test/linalg/diagonal.jl

+    @test LinAlg.mul1!(copy(D1),D2) == D1*D2
+    @test LinAlg.mul2!(D1,copy(D2)) == D1*D2
+    @test LinAlg.mul1!(copy(D1),D2) == D1*D2
+    @test LinAlg.mul2!(D1,copy(D2)) == D1*D2


Three repetitions of the same pair of tests?

Sacha0 · 2018-01-12T00:39:40Z

test/linalg/triangular.jl

@@ -493,6 +494,7 @@ end
 let n = 5
    A = rand(Float16, n, n)
    B = rand(Float16, n-1, n-1)
+


Sacha0 · 2018-01-12T01:14:10Z

Modulo rebase this looks great! Much thanks for your persistence Simon! :)

Summary of sentiments: On the one hand, I sympathize with Andreas that the names mul!, ldiv!, and rdiv! are lovely. On the other hand, I am not as clever as Andreas, so two-argument mul! does trip me up now and again. So overall I favor merging the minimal set of renames necessary to sort the latter (i.e. two-argument mul! -> (mul1!|mul2!) as in this pull request) while leaving everything else as is.

In short, 👍 for these changes (or any spelling variation on mul1!/mul2!). Best!

JeffBezanson · 2018-01-17T04:37:07Z

I think this is ready to go now.

simonbyrne mentioned this pull request Jan 5, 2018

document that 2-arg mul! methods trashes all arguments #16772

Closed

JeffBezanson changed the title ~~Clarify which are is modified in mul/div.~~ Clarify which arg is modified in mul/div. Jan 5, 2018

Sacha0 reviewed Jan 6, 2018

View reviewed changes

simonbyrne force-pushed the sb/mul12 branch from fbed5b4 to 3d9316b Compare January 6, 2018 00:58

Sacha0 reviewed Jan 7, 2018

View reviewed changes

Sacha0 reviewed Jan 12, 2018

View reviewed changes

stdlib/SuiteSparse/src/cholmod.jl Outdated

@@ -1,3 +1,4 @@

Copy link

Member

Sacha0 Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? :)

Sacha0 reviewed Jan 12, 2018

View reviewed changes

test/linalg/triangular.jl Outdated

@@ -493,6 +494,7 @@ end

let n = 5

A = rand(Float16, n, n)

B = rand(Float16, n-1, n-1)

Copy link

Member

Sacha0 Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? :)

simonbyrne force-pushed the sb/mul12 branch from 57f09be to fed0945 Compare January 15, 2018 23:26

JeffBezanson added this to the 1.0 milestone Jan 16, 2018

JeffBezanson force-pushed the sb/mul12 branch from fed0945 to 57fbd18 Compare January 16, 2018 17:37

JeffBezanson force-pushed the sb/mul12 branch from 57fbd18 to 4fd63eb Compare January 16, 2018 21:07

JeffBezanson mentioned this pull request Jan 17, 2018

Base.LinAlg to LinearAlgebra stdlib #25571

Merged

3 tasks

JeffBezanson force-pushed the sb/mul12 branch 2 times, most recently from d84c9cf to cd67a43 Compare January 18, 2018 22:22

Clarify which args are modified in mul!

473d1d4

JeffBezanson force-pushed the sb/mul12 branch from cd67a43 to 473d1d4 Compare January 19, 2018 03:10

JeffBezanson merged commit 6c3cee7 into master Jan 19, 2018

JeffBezanson deleted the sb/mul12 branch January 19, 2018 15:42

		@@ -398,6 +400,7 @@ ldiv!(translu::Transpose{Float64,<:UmfpackLU{Float64}}, B::StridedVecOrMat{<:Com
		ldiv!(adjlu::Adjoint{Float64,<:UmfpackLU{Float64}}, B::StridedVecOrMat{<:Complex}) =
		(lu = adjlu.parent; ldiv!(B, Adjoint(lu), copy(B)))

Clarify which arg is modified in mul/div. #25421

Clarify which arg is modified in mul/div. #25421

Conversation

simonbyrne commented Jan 5, 2018

JeffBezanson commented Jan 5, 2018

Sacha0 left a comment

Choose a reason for hiding this comment

simonbyrne commented Jan 6, 2018

Sacha0 commented Jan 6, 2018

KristofferC commented Jan 6, 2018 • edited Loading

tkelman commented Jan 6, 2018 • edited Loading

simonbyrne commented Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 commented Jan 12, 2018

JeffBezanson commented Jan 17, 2018

KristofferC commented Jan 6, 2018 •

edited

Loading

tkelman commented Jan 6, 2018 •

edited

Loading

simonbyrne commented Jan 6, 2018 •

edited

Loading