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

issymmetric with rtol, atol #36243

Closed
jamblejoe opened this issue Jun 11, 2020 · 8 comments
Closed

issymmetric with rtol, atol #36243

jamblejoe opened this issue Jun 11, 2020 · 8 comments
Labels
domain:linear algebra Linear algebra kind:feature Indicates new feature / enhancement requests

Comments

@jamblejoe
Copy link
Contributor

Hi,
would be great if one could pass arguments to issymmetric, which specifies the absolut and/or relative tolerance between the upper and lower triangle, similar to isapprox(x,y; atol=..., rtol=...).
Thanks!

@StefanKarpinski
Copy link
Sponsor Member

Would this check isapprox(X[i,j], X[j,i], atol=atol, rtol=rtol) for all indices or check that isapprox(X, transpose(X), atol=atol, rtol=rtol). I'm guessing the latter is more correct.

@StefanKarpinski
Copy link
Sponsor Member

cc @stevengj

@stevengj
Copy link
Member

stevengj commented Jun 11, 2020

I think that isapprox(X, transpose(X), atol=atol, rtol=rtol) is definitely more reasonable here than an element-wise test. Probably should also pass through a norm argument.

(Using an approximate-equality test by default in things like Cholesky routines is a tricker question, which seems like it would require some careful analysis. See also the discussion in #10298.)

@stevengj stevengj added domain:linear algebra Linear algebra kind:feature Indicates new feature / enhancement requests labels Jun 11, 2020
@stevengj stevengj changed the title [FeatureRequest] issymmetric with rtol, atol issymmetric with rtol, atol Jun 11, 2020
@jlapeyre
Copy link
Contributor

jlapeyre commented Nov 1, 2021

I wrote a package to try to approach this in a systematic way. You can have a default notion of closeness for a particular method. But, there is a uniform interface allowing they user to specify what they want.
IsApprox.jl

In particular, for issymmetric (as for many other functions), you can choose exact equality, a matrix norm, or element-wise equality (for a quick exit).

https://github.com/jlapeyre/IsApprox.jl/blob/11af339b500ef7ef37735d689f1d95a441360813/src/base_applications.jl#L53-L92

@stevengj
Copy link
Member

stevengj commented Nov 1, 2021

What's the advantage of calling issymmetric(A, Approx(...)) over isapprox(A, A', ...)?

@oscardssmith
Copy link
Member

It should be about 2x faster.

@jlapeyre
Copy link
Contributor

jlapeyre commented Nov 1, 2021

What's the advantage of calling issymmetric(A, Approx(...)) over isapprox(A, A', ...)?

@stevengjl, In this case, the former reduces to the latter. The sequence of calls is

issymmetric(A, Approx())
ishermitian(A, Approx())
isapprox(Approx(), A, adjoint(A))
# and the final call above goes to the following method.
Base.isapprox(a::Union{EachApprox, Approx}, x, y) = isapprox(x, y; pairs(a.kw)...)

(isapprox takes AbstractApprox as the first argument so that it may be used as a replacement in Base without breaking code)

An advantage is that you can drop in EachApprox, instead and get element-wise comparison:

m = rand(1000,1000);
A = m + m' .* (1 + rand() * 1e-12);

julia> @btime IsApprox.issymmetric($A, EachApprox(atol=1e-15))
  8.555 ns (0 allocations: 0 bytes)
false

julia> @btime IsApprox.issymmetric($A, Approx(atol=1e-15))
  3.638 ms (2 allocations: 7.63 MiB)
false

(EachApprox is not a great name, maybe ElementWise is better.)
The code paths diverge above after the call to ishermitian.

The idea is to do this uniformly for everything. For example iszero behaves the same way (that is, as you would expect):

    m = [9.914830625828892e-11 4.473996666408231e-11 3.066184129948095e-11;
         9.914231937308404e-12 7.07270051593276e-11 6.487445728681865e-11;
         9.587109787642197e-11 6.520784660158512e-11 1.2903560723094866e-11]

    @test iszero(m, Approx(atol=2e-10)) # in norm
    @test ! iszero(m, Approx(atol=1e-10))
    @test iszero(m, EachApprox(atol=2e-10)) # element-wise
    @test iszero(m, EachApprox(atol=1e-10))
    @test ! iszero(m) # exact
    @test ! iszero(m, Equal()) # exact

What I really want to avoid is that each of the many predicates is treated separately to add support for closeness in norm and or elementwise, (and as I recently added, up to a global phase) in a different, and ad hoc, way. I wrote this package after seeing exactly this happen in a number of quantum information packages.

@fredrikekre
Copy link
Member

Duplicate of #28885.

(No need to discuss this in two places as is happening now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:linear algebra Linear algebra kind:feature Indicates new feature / enhancement requests
Projects
None yet
Development

No branches or pull requests

6 participants