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

ishermitian/issymmetric should accept optional atol keyword parameter #28885

Open
GiggleLiu opened this issue Aug 24, 2018 · 14 comments
Open

ishermitian/issymmetric should accept optional atol keyword parameter #28885

GiggleLiu opened this issue Aug 24, 2018 · 14 comments
Labels

Comments

@GiggleLiu
Copy link
Contributor

GiggleLiu commented Aug 24, 2018

I was expecting the following code print two trues.

using LinearAlgebra
siz = 8
base_space = qr(randn(Float64, siz, siz)).Q
phases = rand(siz)
signs = Diagonal(phases)
A = base_space*signs*base_space'
println(A ≈ A')
println(ishermitian(A))

Instead, I get true and false.

The ishermitian turns out using ==. Isn't it better to use or add an atol keyword parameter?

function ishermitian(A::AbstractMatrix)

@GiggleLiu
Copy link
Contributor Author

I think this "feature" is not consistent with other julia methods such as eigen.
eigen(A) here will return real eigenvalues, which means the program take A as a hermitian matrix, although it has ~1e-16 error.

@KristofferC
Copy link
Sponsor Member

See the discussion in #10298.

@garrison
Copy link
Sponsor Member

I agree that the default behavior of ishermitian should not be changed (in line with previous discussion), but in my opinion an optional atol keyword argument would be a welcome feature.

@bjarthur
Copy link
Contributor

simple enough to redefine ishermitian yourself if the julia devs have decided not to add a tolerance argument:

julia> import LinearAlgebra: ishermitian

julia> using LinearAlgebra

julia> function ishermitian(A::AbstractMatrix; isapprox=isapprox)
           indsm, indsn = axes(A)
           if indsm != indsn
               return false
           end
           for i = indsn, j = i:last(indsn)
               if !isapprox(A[i,j], adjoint(A[j,i]))
                   return false
               end
           end
           return true
       end
ishermitian (generic function with 14 methods)

julia> ishermitian([1 2; 2.00000000000001 1])
true

@timholy
Copy link
Sponsor Member

timholy commented Oct 12, 2021

It's possibly less of a decision and more that no one has submitted a PR? But I suspect there is little point since lots of other routines will fail or produce surprising results unless it's exact.

@garrison
Copy link
Sponsor Member

It's possibly less of a decision and more that no one has submitted a PR?

If this is the case, it could be clarified by having an open issue regarding this missing feature. This could be accomplished most easily by opening and renaming this one, while perhaps adjusting the top comment (I no longer have the permission to do any of this), or alternatively by having a new issue elsewhere.

Suggested [edited] title: ishermitian should accept optional atol keyword parameter

@garrison
Copy link
Sponsor Member

garrison commented Oct 12, 2021

But I suspect there is little point since lots of other routines will fail or produce surprising results unless it's exact.

In my own personal experience of dealing with quantum mechanics numerically, the most common use case for this missing feature has been as a sanity check that a matrix I have constructed is actually Hermitian before treating it conceptually as such. Without such a check, it is much easier to let a non-physical Hamiltonian or observable slip through.

Often, the matrix I have constructed will be non-Hermitian only by the tiniest amounts (e.g. because floating point addition is non-associative), and I'd like to perform a conceptual test of whether the matrix is "close enough" to Hermitian.

@timholy timholy changed the title ishermitian is too restrictive ishermitian should accept optional atol keyword parameter Oct 12, 2021
@timholy
Copy link
Sponsor Member

timholy commented Oct 12, 2021

Reopened and renamed as suggested.

@timholy timholy reopened this Oct 12, 2021
@bjarthur
Copy link
Contributor

my use case is also as a sanity check to make sure that a numerically simulated matrix that should be symmetric actually is.

@bjarthur
Copy link
Contributor

my re-definition above would be better if the added keyword argument were isequal=isequal and then folks like me and @garrison could use isequal=isapprox. for everyone else nothing changes and completely backward compatible.

@bjarthur
Copy link
Contributor

@garrison see #36243 (comment). isapprox(A,A') suffices for me. hadn't thought of doing that.

@garrison
Copy link
Sponsor Member

Oh, interesting find regarding #36243. I had been thinking recently that anything decided here should apply to issymmetric as well.

@oscardssmith
Copy link
Member

isapprox(A,A') will probably be about 2x slower, but that's probably not a major problem for many use cases.

@jlapeyre
Copy link
Contributor

jlapeyre commented Nov 1, 2021

I wrote a package a while back to try to approach specifying measure of closeness in a uniform way: IsApprox.jl. I included code for many functions/methods including ishermitian.

Four subtypes of AbstractApprox are included: Equal, Approx, EachApprox, and UpToPhase.
IsApprox implements the interface at least partially for each of: isone, iszero, ishermitian, issymmetric, isreal, isinteger, istriu, istril, isbanded, isdiag, isposdef, ispossemidef, isunitary, isinvolution.

@fredrikekre fredrikekre changed the title ishermitian should accept optional atol keyword parameter ishermitian/issymmetric should accept optional atol keyword parameter Nov 1, 2021
@brenhinkeller brenhinkeller added the domain:linear algebra Linear algebra label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants