Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LinearAlgebra] Add isdiag/isposdef for Diagonal and UniformScaling #29638

Merged
merged 10 commits into from Oct 16, 2018

Conversation

4 participants
@dkarrasch
Copy link
Contributor

commented Oct 14, 2018

I found a tiny tiny gap, a noop test for diagonality of Diagonal matrices. It seems that all other matrix structure tests have specialized Diagonal-methods: istriu, istril, issymmetric, and isposdef.

Edit: Based on @mcognetta's suggestion, this now also includes isdiag and isposdef for UniformScalings (title changed accordingly).

dkarrasch added some commits Oct 14, 2018

@mcognetta

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

There should probably be the same for UniformScaling, which currently errors:

julia> isdiag(UniformScaling(2))
ERROR: MethodError: no method matching isdiag(::UniformScaling{Int64})
Closest candidates are:
  isdiag(::AbstractArray{T,2} where T) at /home/mc/github/julia-stable/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/generic.jl:1127
  isdiag(::Number) at /home/mc/github/julia-stable/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/generic.jl:1128
Stacktrace:
 [1] top-level scope at none:0

julia> versioninfo()
Julia Version 1.0.0
Commit 5d4eaca0c9 (2018-08-08 20:58 UTC)

Edit: Also isposdef for UniformScaling. I can put these in another PR if that would be better.

dkarrasch added some commits Oct 15, 2018

@dkarrasch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Thanks @mcognetta for your suggestions. I think I found the right spot to put those methods and went ahead to include them. I hope you don't mind.

@dkarrasch dkarrasch changed the title [LinearAlgebra/diagonal] Add (trivial) isdiag(::Diagonal) [LinearAlgebra] Add (trivial) isdiag(::Diagonal) and isdiag/isposdef(::UniformScaling) Oct 15, 2018

istriu(::UniformScaling) = true
istril(::UniformScaling) = true
issymmetric(::UniformScaling) = true
ishermitian(J::UniformScaling) = isreal(J.λ)
isposdef(J::UniformScaling) = J.λ > zero(J.λ)

This comment has been minimized.

Copy link
@mcognetta

mcognetta Oct 15, 2018

Contributor

This fails when J.λ is complex with imaginary part 0 (since there is no comparison operator).

You can simply do isposdef(J::UniformScaling) = isposdef(J.λ).

@mcognetta

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

@dkarrasch No problem. I had two other tests that might be useful as well (for the case I mentioned in the comment).

    @test isposdef(UniformScaling(complex(1.0, 0.0)))
    @test !isposdef(UniformScaling(complex(1.0, 1.0)))
@test istriu(I)
@test istril(I)
@test issymmetric(I)
@test issymmetric(UniformScaling(complex(1.0,1.0)))
@test ishermitian(I)
@test !ishermitian(UniformScaling(complex(1.0,1.0)))
@test isposdef(I)
@test !isposdef(-I)

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Oct 15, 2018

Member

Maybe test these for complex-valued UniformScalings as well?

This comment has been minimized.

Copy link
@dkarrasch

dkarrasch Oct 15, 2018

Author Contributor

Yes, I think that's what @mcognetta's suggestion above does, I believe. This is now included.

@@ -123,6 +123,7 @@ factorize(D::Diagonal) = D
real(D::Diagonal) = Diagonal(real(D.diag))
imag(D::Diagonal) = Diagonal(imag(D.diag))

isdiag(D::Diagonal) = true

This comment has been minimized.

Copy link
@mcognetta

mcognetta Oct 15, 2018

Contributor

Sorry, one more thing. This fails when we have a block diagonal matrix in which not all blocks are diagonal.

MWE:

julia> D=Diagonal([[1 0; 0 1], [1 0; 1 1]])
2×2 Diagonal{Array{Int64,2},Array{Array{Int64,2},1}}:
 [1 0; 0 1]      ⋅     
     ⋅       [1 0; 1 1]

julia> isdiag(D)
true

I believe it should be
isdiag(D::Diagonal) = all(isdiag, D.diag)
isdiag(D::Diagonal{<:Number}) = true

But I am not so sure on what it means to be a matrix of matrices so maybe @fredrikekre can correct me.

This comment has been minimized.

Copy link
@dkarrasch

dkarrasch Oct 15, 2018

Author Contributor

I see, I only had the Number case in mind. I'll fix this.

This comment has been minimized.

Copy link
@dkarrasch

dkarrasch Oct 15, 2018

Author Contributor

... and add a test.

@dkarrasch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Cool, I'll include them. Thanks!

dkarrasch added some commits Oct 15, 2018

@mcognetta

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

While you are at it, you might as well correct isposdef for diagonals. Right now it checks all(x -> x>0, D.diag) which fails for the same cases as before (complex and block diagonals). A better method is

isposdef(D::Diagonal) = all(isposdef, D.diag)

@dkarrasch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

That wasn't me. :-))) Indeed, isposdef(Diagonal([[1 0; 0 1], [1 0; 0 1]])) was failing for lack of isless.

I included it and will add tests.

@dkarrasch dkarrasch changed the title [LinearAlgebra] Add (trivial) isdiag(::Diagonal) and isdiag/isposdef(::UniformScaling) [LinearAlgebra] Add isdiag/isposdef for Diagonal and UniformScaling Oct 15, 2018

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

This is looking good. Is it ready to go from your perspective, @dkarrasch?

@dkarrasch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Thanks. I think so, yes.

@StefanKarpinski StefanKarpinski merged commit da5f1cc into JuliaLang:master Oct 16, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Compat admonitions and NEWS for Julia 1.1 (#30230)
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.