Deprecate scale! in favor of mul!, mul1!, and mul2! #25701

andreasnoack · 2018-01-23T10:55:02Z

fredrikekre · 2018-01-23T13:11:07Z

stdlib/LinearAlgebra/docs/src/index.md

@@ -354,7 +354,8 @@ LinearAlgebra.tril!
 LinearAlgebra.diagind
 LinearAlgebra.diag
 LinearAlgebra.diagm
-LinearAlgebra.scale!
+LinearAlgebra.mul1!


Perhaps these should be moved to the ## Low-level matrix operations-section with mul!, ldiv! and rdiv! further down (~L433).

fredrikekre · 2018-01-23T13:19:38Z

stdlib/LinearAlgebra/src/deprecated.jl

+@deprecate scale!(A::AbstractMatrix, b::AbstractVector) mul1!(A, Diagonal(b))
+@deprecate scale!(a::AbstractVector, B::AbstractMatrix) mul2!(Diagonal(a), B)
+@deprecate scale!(C::AbstractMatrix, A::AbstractMatrix, b::AbstractVector) mul!(X, A, Diagonal(b))
+@deprecate scale!(C::AbstractMatrix, a::AbstractVector, B::AbstractMatrix) mul!(X, Diagonal(a), B)


X -> C (or C -> X) and same on the line above.

fredrikekre · 2018-01-23T13:23:20Z

stdlib/LinearAlgebra/src/generic.jl


 """
- scale!(A, b)
- scale!(b, A)
+ mul1!(A, b)


Perhaps add ::AbstractArray and ::Number to the signature since there are other mul1! methods?

fredrikekre · 2018-01-23T13:26:01Z

stdlib/LinearAlgebra/src/generic.jl

-(similar to `Diagonal(b)*A`), again operating in-place on `A`. An `InexactError` exception is
-thrown if the scaling produces a number not representable by the element type of `A`,
-e.g. for integer types.
-
 # Examples
 ```jldoctest
 julia> a = [1 2; 3 4]


a -> A to match the signature?

fredrikekre · 2018-01-23T13:26:19Z

stdlib/LinearAlgebra/src/generic.jl

-julia> scale!(b,a)
+# Examples
+```jldoctest
+julia> a = [1 2; 3 4]


a -> A to match the signature?

fredrikekre · 2018-01-23T13:27:10Z

stdlib/LinearAlgebra/src/triangular.jl

-scale!(A::Union{UpperTriangular,LowerTriangular}, c::Number) = scale!(A,A,c)
-scale!(c::Number, A::Union{UpperTriangular,LowerTriangular}) = scale!(A,c)
+mul1!(A::Union{UpperTriangular,LowerTriangular}, c::Number) = mul!(A, A, c)
+mul2!(c::Number, A::Union{UpperTriangular,LowerTriangular}) = mul1!(A', c')'


One too many '.

I think this one is correct: c*A = (c*A)'' = ((c*A)')' = (A'*c')'

Oh, I confused myself and thought the line ended with the char ')'

I'm confused — why can't you simply define mul2!(c::Number, A) = mul1(A, c)?

Oh, I see, it's for non-commutative numbers like quaternions.

Maybe we should have

mul2!(c::Union{Real,Complex}, A::Union{UpperTriangular,LowerTriangular}) = mul1!(A, c)?

if it makes a performance difference in the common case of commutative *.

This one was hitting the generic fallback so it ended up being about twice as slow. I realized a better version would be to call mul!(A, c, A) and define that method. It also made me fix mul!(A, A, c) since it actually computed c*A. Eventually, we should fix the Adjoint/Transpose version so we avoid traversing the zero parts but that is an optimization for a separate PR.

fredrikekre · 2018-01-23T13:41:57Z

stdlib/LinearAlgebra/src/generic.jl

- 2
-
-julia> scale!(a,b)
+julia> mul1!(a, 2)


fredrikekre · 2018-01-23T13:42:13Z

stdlib/LinearAlgebra/src/generic.jl


-julia> b = [1; 2];
+Scale an array `A` by a scalar `b` overwriting `A` in-place.


A -> B twice.

stevengj · 2018-01-24T13:19:50Z

stdlib/LinearAlgebra/src/triangular.jl

+ for j = 1:n
+ @inbounds A[j,j] = c
+ for i = (j + 1):n
+ @inbounds A[i,j] = B[i,j] * c


c * B[i,j] ?

JeffBezanson · 2018-01-24T20:48:41Z

~~Network issue on travis.~~ Now passed. Ready to merge?

stevengj · 2018-01-27T21:48:50Z

I have to say that I find the names mul1! and mul2! mystifying... lmul! and rmul!?

andreasnoack · 2018-01-29T10:09:00Z

I have to say that I find the names mul1! and mul2! mystifying... lmul! and rmul!?

I agree. They are actually the more obvious choices given that we already have ldiv! and rdiv!. I'll prepare a PR unless somebody objects strongly.

StefanKarpinski · 2018-01-29T13:42:12Z

Just to clarify, which argument it mutated by which function? The convention seems to be that a mutating left-division mutates the right argument while a mutating right-division mutates the left argument. Right? Er, I mean correct? So by analogy, lmul!(A, B) should mutate B while rmul!(A, B) should mutate A. This is probably what you're doing, @andreasnoack, but just checking.

martinholters · 2018-01-29T13:53:37Z

That is very satisfyingly consistent with ldiv! and rdiv! ... but ... if you're not used to these, it's not too obvious what these muls do.

StefanKarpinski · 2018-01-29T14:01:04Z

but ... if you're not used to these

These aren't exactly the most mainstream functions so I think that "satisfying consistency" with another pair of niche in-place operators is as good as we can hope for. It is a very satisfying symmetry and kind of interesting since \ and / are different but there's only one * – which doesn't matter in the non-mutating case but does matter in the mutating case. The conclusion seems to be that for division you need separate left and right operators even without mutation but for multiplication you can gloss over that distinction... except in a mutating API, for which you need separate left and right multiplication operators.

andreasnoack · 2018-01-29T14:07:20Z

These aren't exactly the most mainstream functions so I think that "satisfying consistency" with another pair of niche in-place operators is as good as we can hope for.

Yes, and it is also not obvious which argument mul1! mutates so most users would have to consult the documentation in any case. Furthermore, the l and r would be similar to the side argument of xtrmm in BLAS.

martinholters · 2018-01-29T15:47:39Z

Thinking about this again, I find the consistency not so satisfying anymore: Usually func! is the mutating version of func, and the first argument is the one being mutated. Now ldiv! and rdiv! correspond to \ and / for the lack of all-letter-aliases of those. Oh, and one of them mutates its second argument. (Quick, without looking, which one?) But mul1! and mul2! both correspond to *. We don't distinguish "multiplying A with B from the right" and "multiplying B with A from the left". Should we?

Maybe we should rename ldiv! to ldiv2!? (Not really, but that would be the right king of consistency.)

StefanKarpinski · 2018-01-29T16:34:27Z

The key word above is "usually". Mutating function mutate their "subject" for some appropriate notion of subject, which is usually the first argument, but not always. In the case of left division, the subject is being divided from the left, so the subject is the right argument. In the case of right division, the subject is being divided from the right, so the subject is the left argument. The same logic applies to lmul! and rmul!. There is no need for left and right multiplication operators because the operator form is non-mutating and without mutation, it doesn't matter which argument is the "subject" – the end result is the same.

Sacha0 · 2018-01-29T18:55:55Z

I can appreciate the argument for renaming mul1!/mul2! to rmul!/lmul!. I can also appreciate an argument for doing the same and then swapping the argument order of ldiv! and lmul!, in that the description each such function is then "in-place (multiplication|division) of the first argument by, on the (left|right), the second argument". Three-argument ldiv! methods perhaps introduce a wrinkle in the latter however. The additional wrinkle I see is generalization to triple and higher products; the numbering approach scales, whereas the the l/r approach does not immediately appear to? Best!

StefanKarpinski · 2018-01-30T14:21:29Z

I don't know where the notion that the first argument should always the one to be mutated came from. That's neither empirically true in our existing APIs, nor a hard-and-fast principle that we've ever aimed to adhere to. It is often the case, but in cases where it doesn't make sense, we don't do that. This seems like one of those cases.

Sacha0 · 2018-01-30T20:20:50Z

It is often the case, but in cases where it doesn't make sense, we don't do that. This seems like one of those cases.

Playing devil's advocate (and noting that either argument order seems fine on this end), the perspective from which the mutated-argument-first order seems natural to me is the verb!(subject, object) convention (as in "left-/right-multiply subject by object"). Of course this convention is not objectively privileged, but it has grown natural to me given its prevalence. The triple and higher product concern is more objectively meaningful, but that's a bridge we can cross when we get to it :). Best!

StefanKarpinski · 2018-01-30T21:37:51Z

Writing any mutating version of A*B as xmul!(B, A) does not pass the sniff test to me. It does not seem that hard to remember that lmul!(A, B) mutates B. We're doing that for ldiv!(A, B) which has not presented any problems until we started overthinking it because of mul!.

Sacha0 · 2018-01-31T20:52:40Z

It does not seem that hard to remember that lmul!(A, B) mutates B. We're doing that for ldiv!(A, B) which has not presented any problems until we started overthinking it because of mul!.

This point does not seem so cut and dry. Among other data points, #25701 (comment) demonstrates that e.g. ldiv!/rdiv! argument order does indeed trip up highly intelligent, experienced parties. Best!

andreasnoack force-pushed the anj/scale branch from 0139478 to cb8f50f Compare January 23, 2018 11:03

fredrikekre reviewed Jan 23, 2018

View reviewed changes

andreasnoack force-pushed the anj/scale branch from e254310 to 87abe97 Compare January 23, 2018 13:45

JeffBezanson added this to the 1.0 milestone Jan 23, 2018

andreasnoack added 4 commits January 23, 2018 22:12

Deprecate scale! in favor of mul!, mul1!, and mul2!

59cc944

Add NEWS entry for scale! deprecation

3e830f9

Import Diagonal to IterativeEigensolvers

f70e600

Fix typos

3f01891

andreasnoack force-pushed the anj/scale branch from 87abe97 to 3f01891 Compare January 23, 2018 21:12

stevengj reviewed Jan 24, 2018

View reviewed changes

Define scaling of triagular matrix from the right and the left

d0ce71a

andreasnoack force-pushed the anj/scale branch from 0ed5488 to d0ce71a Compare January 24, 2018 13:28

JeffBezanson merged commit cf6303a into master Jan 24, 2018

JeffBezanson deleted the anj/scale branch January 24, 2018 23:08

andreasnoack mentioned this pull request Jan 30, 2018

Change mul1! and mul2! to rmul! and lmul! #25812

Merged

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

fredrikekre mentioned this pull request May 16, 2018

Add rmul! JuliaLang/Compat.jl#546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate scale! in favor of mul!, mul1!, and mul2! #25701

Deprecate scale! in favor of mul!, mul1!, and mul2! #25701

andreasnoack commented Jan 23, 2018

fredrikekre Jan 23, 2018

fredrikekre Jan 23, 2018

fredrikekre Jan 23, 2018

fredrikekre Jan 23, 2018

fredrikekre Jan 23, 2018

fredrikekre Jan 23, 2018

andreasnoack Jan 23, 2018

fredrikekre Jan 23, 2018

stevengj Jan 23, 2018 •

edited

Loading

stevengj Jan 23, 2018 •

edited

Loading

andreasnoack Jan 24, 2018

fredrikekre Jan 23, 2018

fredrikekre Jan 23, 2018

stevengj Jan 24, 2018

JeffBezanson commented Jan 24, 2018 •

edited

Loading

stevengj commented Jan 27, 2018

andreasnoack commented Jan 29, 2018

StefanKarpinski commented Jan 29, 2018

martinholters commented Jan 29, 2018

StefanKarpinski commented Jan 29, 2018 •

edited

Loading

andreasnoack commented Jan 29, 2018

martinholters commented Jan 29, 2018

StefanKarpinski commented Jan 29, 2018 •

edited

Loading

Sacha0 commented Jan 29, 2018

StefanKarpinski commented Jan 30, 2018

Sacha0 commented Jan 30, 2018

StefanKarpinski commented Jan 30, 2018

Sacha0 commented Jan 31, 2018


		julia> b = [1; 2];
		Scale an array `A` by a scalar `b` overwriting `A` in-place.

Deprecate scale! in favor of mul!, mul1!, and mul2! #25701

Deprecate scale! in favor of mul!, mul1!, and mul2! #25701

Conversation

andreasnoack commented Jan 23, 2018

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

stevengj Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

stevengj Jan 23, 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

JeffBezanson commented Jan 24, 2018 • edited Loading

stevengj commented Jan 27, 2018

andreasnoack commented Jan 29, 2018

StefanKarpinski commented Jan 29, 2018

martinholters commented Jan 29, 2018

StefanKarpinski commented Jan 29, 2018 • edited Loading

andreasnoack commented Jan 29, 2018

martinholters commented Jan 29, 2018

StefanKarpinski commented Jan 29, 2018 • edited Loading

Sacha0 commented Jan 29, 2018

StefanKarpinski commented Jan 30, 2018

Sacha0 commented Jan 30, 2018

StefanKarpinski commented Jan 30, 2018

Sacha0 commented Jan 31, 2018

stevengj Jan 23, 2018 •

edited

Loading

stevengj Jan 23, 2018 •

edited

Loading

JeffBezanson commented Jan 24, 2018 •

edited

Loading

StefanKarpinski commented Jan 29, 2018 •

edited

Loading

StefanKarpinski commented Jan 29, 2018 •

edited

Loading