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

Fix real matrix sqrt and log for edge cases #40144

Merged
merged 15 commits into from
Apr 5, 2021

Conversation

sethaxen
Copy link
Contributor

@sethaxen sethaxen commented Mar 22, 2021

Fixes JuliaLang/LinearAlgebra.jl#825, also eliminates an unnecessary scalar sqrt tries to reduce potential numerical issues in _sqrt_real_2x2!.

The added tests fail on master. Test 1 fails unless we make this change to _sqrt_real_2x2!. Test 2 also fails unless we make this change to _sqrt_quasitriu_offdiag_block_2x2!.

Edit: Test 3 tests the case where NaNs would cause LAPACK.trsyl! to error.

@oscardssmith
Copy link
Member

How does this affect the performance?

@sethaxen
Copy link
Contributor Author

How does this affect the performance?

This version of _sqrt_real_2x2! takes about 5ns longer than on master (~35ns on master, ~40ns here). Using the same benchmark code and matrices as in #39973 (comment), sqrt_quasitriu takes about 16.5 μs on master and 17.2μs here.

@stevengj stevengj added the linear algebra Linear algebra label Mar 23, 2021
@sethaxen
Copy link
Contributor Author

sethaxen commented Mar 23, 2021

With these changes, _sqrt_real_2x2! is ~33% slower. That being said, on a 20x20 matrix, sqrt_quasitriu is only ~4% slower. sqrt overflows and underflows much less with these changes, so I think this decrease in performance is worth it.

Updated Benchmarks

Setup:

using BenchmarkTools, LinearAlgebra
using Random; Random.seed!(1)
F = schur(randn(20, 20)^2)
x = F.T[1:2,1:2] # 2x2 block
y = zero(x)

Before:

julia> @btime $(LinearAlgebra._sqrt_real_2x2!)($y, $x);
  35.130 ns (0 allocations: 0 bytes)
 
julia> @btime $(LinearAlgebra.sqrt_quasitriu)($(F.T));
  16.880 μs (1 allocation: 3.25 KiB)

This PR:

julia> @btime $(LinearAlgebra._sqrt_real_2x2!)($y, $x);
  47.072 ns (0 allocations: 0 bytes)

julia> @btime $(LinearAlgebra.sqrt_quasitriu)($(F.T));
  17.599 μs (1 allocation: 3.25 KiB)

@sethaxen
Copy link
Contributor Author

The 2x2 diagonal block computation of log suffered from the same overflow/underflow, so I also adopted a similar approach for that.

@sethaxen
Copy link
Contributor Author

With the recent changes, _sqrt_real_2x2! is almost as fast as the original:

julia> @btime $(LinearAlgebra._sqrt_real_2x2!)($y, $x);
  38.129 ns (0 allocations: 0 bytes)

julia> @btime $(LinearAlgebra.sqrt_quasitriu)($(F.T));
  17.354 μs (1 allocation: 3.25 KiB)

@sethaxen sethaxen changed the title Fix real sqrt for edge cases Fix real matrix sqrt and log for edge cases Mar 24, 2021
@stevengj stevengj added the bugfix This change fixes an existing bug label Mar 24, 2021
@musm
Copy link
Contributor

musm commented Mar 24, 2021

This is a bugfix so this probably needs to be backported?

@sethaxen
Copy link
Contributor Author

This is a bugfix so this probably needs to be backported?

No, this fixes bugs introduced in #39973. The new tests added here, which fail on master, pass on Julia 1.6.

@musm musm merged commit b780905 into JuliaLang:master Apr 5, 2021
@musm
Copy link
Contributor

musm commented Apr 5, 2021

Merging, as this has been approved by @stevengj and has had enough time for any objections or comments since then.

@sethaxen sethaxen deleted the fixquasisqrt branch April 5, 2021 19:05
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* Add failing tests

* Improve 2x2 real sqrt for scalar diagonal

* Avoid sylvester if zero is a solution

* Avoid unnecessary sqrt

* Use correct variable names

* Avoid erroring due to NaNs

* Exactly adapt Higham's algorithm, with hypot

* Remove unreachable branch

* Add failing tests

* Handle overflow/underflow

* Invert instead of dividing

* Apply d=0 constraint from standardized real schur form

* Handle underflow

* Avoid underflow/overflow in log diagonal blocks

* Add tests for log underflow/overflow
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* Add failing tests

* Improve 2x2 real sqrt for scalar diagonal

* Avoid sylvester if zero is a solution

* Avoid unnecessary sqrt

* Use correct variable names

* Avoid erroring due to NaNs

* Exactly adapt Higham's algorithm, with hypot

* Remove unreachable branch

* Add failing tests

* Handle overflow/underflow

* Invert instead of dividing

* Apply d=0 constraint from standardized real schur form

* Handle underflow

* Avoid underflow/overflow in log diagonal blocks

* Add tests for log underflow/overflow
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
* Add failing tests

* Improve 2x2 real sqrt for scalar diagonal

* Avoid sylvester if zero is a solution

* Avoid unnecessary sqrt

* Use correct variable names

* Avoid erroring due to NaNs

* Exactly adapt Higham's algorithm, with hypot

* Remove unreachable branch

* Add failing tests

* Handle overflow/underflow

* Invert instead of dividing

* Apply d=0 constraint from standardized real schur form

* Handle underflow

* Avoid underflow/overflow in log diagonal blocks

* Add tests for log underflow/overflow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Real matrix sqrt fails for specific quasi-triangular matrix
5 participants