Add some (matrix) functions for UniformScaling #28872

ZeppLu · 2018-08-24T12:56:32Z

Notations used here:

I: identity matrix, known as UniformScaling in julia
x: scalar
v: column vector
M: matrix

Methods list:

tests not yet written

mbauman · 2018-08-24T15:21:27Z

This is great — welcome! The failures look to be due to missing imports (or Base. qualifications) of the matrix functions from base.

In fact none of other matrices in LinearAlgebra specializes cbrt(), I saw sqrt() and then added cbrt() as well without ever thinking.

changes: M = ... to: M(x) = ... in order to make such conversion reusable in following tests

I forgot it...

ZeppLu · 2018-08-25T14:47:36Z

I'm having trouble understanding

julia/stdlib/LinearAlgebra/src/uniformscaling.jl

Lines 175 to 178 in 16dcabe

    
           \(A::Union{Bidiagonal{T},AbstractTriangular{T}}, J::UniformScaling) where {T<:Number} = 
        
               rmul!(inv(A), J.λ) 
        
           \(J::UniformScaling, A::AbstractVecOrMat) = J.λ == 0 ? throw(SingularException(1)) : J.λ\A 
        
           \(A::AbstractMatrix, J::UniformScaling) = rmul!(inv(A), J.λ)

IIUC, line 175-176 is redundant, since Union{Bidiagonal{T},AbstractTriangular{T}} has already been covered by AbstractMatrix. Or is there any special cases that should be paid attention to?

andreasnoack · 2018-08-27T07:19:56Z

My guess is that they are there to avoid ambiguities. You can try to comment them out and see what happens.

`J` is used by following tests, but I mistakenly changed its value

I made v a scalar, forgive me

`Union{Bidiagonal,AbstractTriangular} \ UniformScaling` seems redundant because there's another method `AbstractMatrix \ UniformScaling`

ZeppLu · 2018-08-27T13:15:52Z

Just did a git log -L to find out the first commit that adds this line, which is done by @andreasnoack ! There surely are times a programmer forgets why he wrote a piece of code, and I come across the same feeling now and then, hahaha

also made `isapprox` a standalone testset, because it's irrelevant to `istriu`, `issymmetric`, etc

andreasnoack · 2018-08-29T10:15:39Z

Hi ZeppLu. It's great that you are working on this. However, it would be great if you could push your changes a little less frequently since each time you push changes, it triggers new CI runs.

ZeppLu · 2018-08-30T01:45:33Z

This is my first time working with a CI, apologize for any troubles I've made and thanks for your advice!

dalum · 2018-08-31T08:51:34Z

Looks good! Maybe you could also add ^(J::UniformScaling, M::Matrix) = exp!(log(J.λ)*M). On that note, maybe ^(x::Number, M::Matrix) = exp!(log(x)*M) should probably be added first? Just a thought. 🙂

ZeppLu · 2018-09-02T16:07:08Z

Adding ^(x::Number, M::Matrix) and exp! to stdlib is beyond the purpose of this PR, which only focuses on UniformScaling. I'd suggest you open another PR.

BTW, I'm going to be quite busy this month (Sep), and will be back here as soon as I'm free again.

dkarrasch · 2019-03-26T08:55:47Z

stdlib/LinearAlgebra/src/uniformscaling.jl

 *(J1::UniformScaling, J2::UniformScaling) = UniformScaling(J1.λ*J2.λ)
 *(B::BitArray{2}, J::UniformScaling) = *(Array(B), J::UniformScaling)
 *(J::UniformScaling, B::BitArray{2}) = *(J::UniformScaling, Array(B))
-*(A::AbstractMatrix, J::UniformScaling) = A*J.λ
+*(A::AbstractVecOrMat, J::UniformScaling) = A*J.λ


I don't think this is well-defined for vectors. If we think of J being the λ-fold of the identity matrix, the multiplying a vertical vector from the left is meaningless.

Sorry my linear algebra class was not taught in English, I can't get what "the λ-fold of the identity matrix" means, could you explain it in detail?

It's just saying that it's some identity matrix multiplied by λ.

This operation, multiplying a vec*mat is frequently an error. I typically think of an identity matrix as being larger than 1x1, so I initially agreed. But this is well-defined if J is considered to be 1x1... and we do say it automatically infers its size based upon the operation at hand.

I do think some extra consideration is still needed here, though, as multiplying rand(5) * fill(2, 1, 1) yields a 5×1 matrix and not a 5-element vector (as it would return as it's currently defined).

Yes, the issue is the distinction between a simple vector v and the linear map x \mapsto v*x. In the first case, v should be a vector as we know it. In the second, v should be an nx1 matrix. Take your example rand(5) * fill(2, 1, 1). As it stands, it is non-sensical. Checking with @which what is actually done leads to

julia/stdlib/LinearAlgebra/src/matmul.jl

Line 62 in d60503a

(*)(a::AbstractVector, B::AbstractMatrix) = reshape(a,length(a),1)*B

You see, to perform the product you first need to reshape the vector into a matrix, and only then multiplication works as expected. This and the lines above the quote are somewhat strange "convenience" functions, I would say. Anyway, if we wanted to be consistent with what we already have, we should have

*(A::AbstractMatrix, J::UniformScaling) = A*J.λ *(a::AbstractVector, J::UniformScaling) = reshape(a*J.λ, length(a), 1)

You'll probably enjoy the discussion in #4774 if you've not discovered that one yet. In short, we've decided to allow vectors to participate in the linear algebra of matrices — and in many cases that means they behave as though they have an implicit trailing ×1 dimension. This particular case is covered in #20423. Sure it's implemented as a reshape, but the important part is the behavior — and this is the behavior we've decided to have.

Thanks @mbauman, for the link. I remember reading it some long time ago. I agree it's a design decision, and I imagined that behavior is desired. Would

*(a::AbstractVector, J::UniformScaling) = reshape(a, length(a), 1) * J

be then more appropriate? We would leave the multiplication handling to more generic parts of the code.

Thanks you two! I also consider *(a::AbstractVector, J::UniformScaling) = reshape(a, length(a), 1) * J more reasonable

dkarrasch · 2019-03-26T09:06:13Z

stdlib/LinearAlgebra/src/uniformscaling.jl

 *(J::UniformScaling, A::AbstractVecOrMat) = J.λ*A
 *(x::Number, J::UniformScaling) = UniformScaling(x*J.λ)
 *(J::UniformScaling, x::Number) = UniformScaling(J.λ*x)

 /(J1::UniformScaling, J2::UniformScaling) = J2.λ == 0 ? throw(SingularException(1)) : UniformScaling(J1.λ/J2.λ)
 /(J::UniformScaling, A::AbstractMatrix) = lmul!(J.λ, inv(A))
-/(A::AbstractMatrix, J::UniformScaling) = J.λ == 0 ? throw(SingularException(1)) : A/J.λ
+/(A::AbstractVecOrMat, J::UniformScaling) = J.λ == 0 ? throw(SingularException(1)) : A/J.λ


I think this should remain AbstractMatrix for the same reason as above.

dkarrasch · 2019-03-26T09:13:24Z

stdlib/LinearAlgebra/src/uniformscaling.jl

@@ -79,11 +81,15 @@ istril(::UniformScaling) = true
 issymmetric(::UniformScaling) = true
 ishermitian(J::UniformScaling) = isreal(J.λ)

+isposdef(J::UniformScaling) = isreal(J.λ) && J.λ > 0


How about

isposdef(J::UniformScaling) = isposdef(J.λ)

?

you're right!

- *(v::AbstractVector, J::UniformScaling) - /(v::AbstractVector, J::UniformScaling) - isposdef(J::UniformScaling)

stdlib/LinearAlgebra/test/uniformscaling.jl

stdlib/LinearAlgebra/src/uniformscaling.jl

chriscoey · 2019-08-21T18:35:52Z

hey @ZeppLu, thanks for doing this, any chance you could finish it off?

chriscoey · 2019-08-21T18:36:35Z

also, could this fix the broadcasting with I issue at #23197?

dkarrasch · 2020-02-19T13:51:50Z

Anyone care to take a quick look? @jagot @JeffreySarnoff ?

jagot · 2020-02-19T14:22:09Z

I think it looks fine!

vtjnash · 2020-02-19T19:29:19Z

This introduced some build issues:

WARNING: Method definition isposdef(LinearAlgebra.UniformScaling{T} where T<:Number) in module LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/uniformscaling.jl:109 overwritten at /data/vtjnash/julia/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/uniformscaling.jl:111.
WARNING: Method definition ^(LinearAlgebra.UniformScaling{T} where T<:Number, Number) in module LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/uniformscaling.jl:118 overwritten at /data/vtjnash/julia/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/uniformscaling.jl:269.
WARNING: Method definition broadcasted(typeof(Base.:(^)), LinearAlgebra.UniformScaling{T} where T<:Number, Number) in module LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/uniformscaling.jl:267 overwritten at /data/vtjnash/julia/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/uniformscaling.jl:272.

dkarrasch · 2020-02-19T20:09:40Z

Sorry! Fix in #34819.

Co-authored-by: Daniel Karrasch <Daniel.Karrasch@gmx.de>

ZeppLu added 3 commits August 24, 2018 20:21

α .\ I, and test

1c9da4c

I ^ α, I .^ α, and tests

721df9d

exp/log, sqrt/cbrt, and trigonometric functions

f51f5f6

tests not yet written

mbauman added the linear algebra Linear algebra label Aug 24, 2018

ZeppLu added 2 commits August 24, 2018 23:45

cbrt(I) removed, since cube root of matrix is rarely needed

27d344d

In fact none of other matrices in LinearAlgebra specializes cbrt(), I saw sqrt() and then added cbrt() as well without ever thinking.

tests for exp, log, trigonometric functions on \bbC

f8dc3b1

peterkinalex approved these changes Aug 25, 2018

View reviewed changes

ZeppLu added 3 commits August 25, 2018 21:10

minor update

53f8a3b

changes: M = ... to: M(x) = ... in order to make such conversion reusable in following tests

tests for exp, log, trigonometric functions on \bbR

cd11f04

add tests for sqrt

16dcabe

I forgot it...

v * I, and tests

762b39b

ZeppLu added 4 commits August 27, 2018 16:12

fix tests

6dfa184

`J` is used by following tests, but I mistakenly changed its value

v / I, and tests

56025a6

fix tests, seriously

8be120a

I made v a scalar, forgive me

remove possibly redundant code to see if tests fail

c66ce05

`Union{Bidiagonal,AbstractTriangular} \ UniformScaling` seems redundant because there's another method `AbstractMatrix \ UniformScaling`

ZeppLu added 5 commits August 29, 2018 09:49

real, imag, and tests

c600d52

isposdef, and tests

dc2eeb3

also made `isapprox` a standalone testset, because it's irrelevant to `istriu`, `issymmetric`, etc

tr, and tests

5991853

rearrange test code to make it more concise

fb7fb25

reduce size of matrix used for testing to reduce test time

a642feb

ZeppLu force-pushed the I-methods branch from 0340078 to a642feb Compare August 29, 2018 06:05

pinv, and tests

2c63d67

dkarrasch reviewed Mar 26, 2019

View reviewed changes

ZeppLu added 4 commits March 29, 2019 15:13

update according to reviewers' suggestions

48cfc4d

- *(v::AbstractVector, J::UniformScaling) - /(v::AbstractVector, J::UniformScaling) - isposdef(J::UniformScaling)

Merge branch 'master' into I-methods

3438fe3

fix test

1785492

fix test (element-wise euqality)

d659b40

dkarrasch reviewed Apr 4, 2019

View reviewed changes

stdlib/LinearAlgebra/test/uniformscaling.jl Show resolved Hide resolved

stdlib/LinearAlgebra/src/uniformscaling.jl Show resolved Hide resolved

stdlib/LinearAlgebra/src/uniformscaling.jl Show resolved Hide resolved

stdlib/LinearAlgebra/src/uniformscaling.jl Outdated Show resolved Hide resolved

dkarrasch mentioned this pull request Feb 18, 2020

Matrix functions for UniformScaling #34799

Open

17 tasks

dkarrasch added 2 commits February 19, 2020 08:58

Merge branch 'master' into I-methods

ccb8c41

relax some multiplication/division tests

6cc47a7

dkarrasch merged commit 4188b2c into JuliaLang:master Feb 19, 2020

dkarrasch changed the title ~~[WIP] Complete missing methods for UniformScaling~~ Add some (matrix) functions for UniformScaling Feb 19, 2020

dkarrasch added a commit that referenced this pull request Feb 19, 2020

fix build issues introduced in #28872

cea2159

vtjnash pushed a commit that referenced this pull request Feb 20, 2020

fix build issues introduced in #28872 (#34819)

0d39976

birm pushed a commit to birm/julia that referenced this pull request Feb 22, 2020

Add some (matrix) functions for UniformScaling (JuliaLang#28872)

77930fb

Co-authored-by: Daniel Karrasch <Daniel.Karrasch@gmx.de>

birm pushed a commit to birm/julia that referenced this pull request Feb 22, 2020

fix build issues introduced in JuliaLang#28872 (JuliaLang#34819)

3f30a05

KristofferC pushed a commit that referenced this pull request Apr 11, 2020

Add some (matrix) functions for UniformScaling (#28872)

549a09c

Co-authored-by: Daniel Karrasch <Daniel.Karrasch@gmx.de>

KristofferC pushed a commit that referenced this pull request Apr 11, 2020

fix build issues introduced in #28872 (#34819)

b657cbf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some (matrix) functions for UniformScaling #28872

Add some (matrix) functions for UniformScaling #28872

ZeppLu commented Aug 24, 2018 •

edited

Loading

mbauman commented Aug 24, 2018

ZeppLu commented Aug 25, 2018

andreasnoack commented Aug 27, 2018

ZeppLu commented Aug 27, 2018

andreasnoack commented Aug 29, 2018

ZeppLu commented Aug 30, 2018

dalum commented Aug 31, 2018 •

edited

Loading

ZeppLu commented Sep 2, 2018 •

edited

Loading

dkarrasch Mar 26, 2019

ZeppLu Mar 28, 2019

mbauman Mar 28, 2019

dkarrasch Mar 28, 2019

mbauman Mar 28, 2019 •

edited

Loading

dkarrasch Mar 29, 2019 •

edited

Loading

ZeppLu Mar 29, 2019

dkarrasch Mar 26, 2019

dkarrasch Mar 26, 2019

ZeppLu Mar 28, 2019

chriscoey commented Aug 21, 2019

chriscoey commented Aug 21, 2019

dkarrasch commented Feb 19, 2020

jagot commented Feb 19, 2020

vtjnash commented Feb 19, 2020

dkarrasch commented Feb 19, 2020

Add some (matrix) functions for UniformScaling #28872

Add some (matrix) functions for UniformScaling #28872

Conversation

ZeppLu commented Aug 24, 2018 • edited Loading

mbauman commented Aug 24, 2018

ZeppLu commented Aug 25, 2018

andreasnoack commented Aug 27, 2018

ZeppLu commented Aug 27, 2018

andreasnoack commented Aug 29, 2018

ZeppLu commented Aug 30, 2018

dalum commented Aug 31, 2018 • edited Loading

ZeppLu commented Sep 2, 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

mbauman Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

dkarrasch Mar 29, 2019 • 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

chriscoey commented Aug 21, 2019

chriscoey commented Aug 21, 2019

dkarrasch commented Feb 19, 2020

jagot commented Feb 19, 2020

vtjnash commented Feb 19, 2020

dkarrasch commented Feb 19, 2020

ZeppLu commented Aug 24, 2018 •

edited

Loading

dalum commented Aug 31, 2018 •

edited

Loading

ZeppLu commented Sep 2, 2018 •

edited

Loading

mbauman Mar 28, 2019 •

edited

Loading

dkarrasch Mar 29, 2019 •

edited

Loading