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

Add missing constructors for Bidiagonal, Symmetric, and Hermitian #42467

Closed

Conversation

mcognetta
Copy link
Contributor

@mcognetta mcognetta commented Oct 2, 2021

Bidiagonal, Symmetric, and Hermitian are currently all missing the X(A::Matrix, uplo::Char) constructor but have the X(A::Matrix, uplo::Symbol) constructor. This PR adds the Char constructor.

Two additional methods Symmetric(A::AbstractArray) = Symmetric(A, :U) and Hermitian(A::AbstractMatrix) = Hermitian(A, :U) were also added to avoid the method overwriting warning (due to the previous implementation having default arguments) and to preserve the historical default argument.

NOTE: This PR should be submitted after #42466, which adds better uplo checks to the constructors.


HEAD:

julia> Bidiagonal(rand(3,3), :U)
3×3 Bidiagonal{Float64,Array{Float64,1}}:
 0.971135  0.423251    
          0.528393  0.696667
                   0.343728

julia> Bidiagonal(rand(3,3), 'U')
ERROR: MethodError: no method matching Bidiagonal(::Array{Float64,2}, ::Char)
Closest candidates are:
  Bidiagonal(::AbstractArray{T,2} where T, ::Symbol) at /home/mc/github/julia/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/bidiag.jl:101
Stacktrace:
 [1] top-level scope at REPL[3]:1

This PR:

julia> Bidiagonal(rand(3,3), :U)
3×3 Bidiagonal{Float64, Vector{Float64}}:
 0.87165  0.860882    
         0.539619  0.278167
                  0.4687

julia> Bidiagonal(rand(3,3), 'U')
3×3 Bidiagonal{Float64, Vector{Float64}}:
 0.499547  0.862715    
          0.556194  0.751918
                   0.706695

and likewise for Symmetric, and Hermitian.

@mcognetta
Copy link
Contributor Author

Failures look unrelated.

@N5N3
Copy link
Member

N5N3 commented Oct 2, 2021

  1. If Char constructor is introduced to Hermitian, you'd better to add it to hermitian to keep consistency.
  2. Symbol constructor checks :L, :U via char_uplo, so I think we should do similar check in Char constructor.
  3. As for Symmetric(A::AbstractArray) = Symmetric(A, :U), since
julia> f(a,b::Int = 1) = 1
f (generic function with 2 methods)
julia> f(a,b::Float64) = 2
f (generic function with 3 methods)
julia> f(1,1), f(1), f(1,1.0)
(1, 1, 2)

I think you only need to add the new constructor?
4. I also wonder why you say it's missing. Can you give us an example that Char constructor is better than Symbol constructor?

@mcognetta
Copy link
Contributor Author

  1. If Char constructor is introduced to Hermitian, you'd better to add it to hermitian to keep consistency.

Done. Actually, this and symmetric aren't exported. Perhaps they should be.

  1. Symbol constructor checks :L, :U via char_uplo, so I think we should do similar check in Char constructor.

This is done in #42466. I will add a note not to submit this one until that is merged.

  1. As for Symmetric(A::AbstractArray) = Symmetric(A, :U), since
julia> f(a,b::Int = 1) = 1
f (generic function with 2 methods)
julia> f(a,b::Float64) = 2
f (generic function with 3 methods)
julia> f(1,1), f(1), f(1,1.0)
(1, 1, 2)

I think you only need to add the new constructor?

Doing it this way causes a Method Overwriting warning during compilation (doesn't happen in the REPL I guess, but it happens when running make for example.). This discusses it (but, at the time, the warning would still appear in REPL. the same code does not produce the warning in REPL at HEAD):
https://discourse.julialang.org/t/method-definition-overwritten-warning-for-functions-with-default-arguments/1710

  1. I also wonder why you say it's missing. Can you give us an example that Char constructor is better than Symbol constructor?

It's missing in the sense that some of the constructors allow Symbol and Char but some don't. I don't think Char is better, but adding those constructors makes the API more consistent.


Thanks for the comments.

@mcognetta
Copy link
Contributor Author

Note that this should be submitted after #42466, so that the correct uplo checks are included in the base constructors.

@dkarrasch dkarrasch added the linear algebra Linear algebra label Oct 5, 2021
@dkarrasch
Copy link
Member

You can rebase onto current master now.

Comment on lines +145 to +147
# suppress method overwritten warning while preserving
# the historical default argument `uplo=:U`
Hermitian(A::AbstractMatrix) = Hermitian(A, :U)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a new issue or has it always been there but we ignored the warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is new if we add another method with a different default parameter type. The warning was not there before because there was only one Hermatian function with a default type parameter.

@dkarrasch
Copy link
Member

I'm not sure, but I think not exporting symmetric and hermitian was done on purpose. Unless we have a good reason, I wouldn't export them, because you can do what you want by calling the wrapper constructors with capitalized initial letters.

@andreasnoack
Copy link
Member

It's on purpose that we don't have Char constructors. The idea was that Char was just the internal representation whereas Symbol was the public API.

@mcognetta
Copy link
Contributor Author

It's on purpose that we don't have Char constructors. The idea was that Char was just the internal representation whereas Symbol was the public API.

Oh, I see. In that case, should I close this PR?

@mcognetta mcognetta closed this Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants