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

RFC: treat small negative λ as 0 for sqrt(::Hermitian) #35057

Merged
merged 4 commits into from
May 7, 2020

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Mar 9, 2020

This PR treats sufficiently small negative eigenvalues as "zero" for the purpose of taking matrix sqrt and log, as otherwise one gets spurious complex results from semidefinite matrices due to roundoff errors. (See also discourse discussion.)

It works by adding an rtol keyword parameter to sqrt(::Hermitian) and log(::Hermitian), specifying a relative tolerance for negative eigenvalues to ignore, which defaults to the same relative tolerance that we use for the pinv function.

This is currently just a rough draft to get reactions. To do:

  • tests
  • documentation
  • news
  • remove rtol argument for log, where semidefinite matrices don't make sense

@stevengj stevengj added linear algebra Linear algebra needs tests Unit tests are required for this change needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Mar 9, 2020
@stevengj stevengj requested a review from andreasnoack March 9, 2020 21:17
@stevengj stevengj changed the title RFC: treat small negative λ as 0 for sqrt(::Hermitian) and log(::Hermitian) RFC: treat small negative λ as 0 for sqrt(::Hermitian) Mar 10, 2020
@stevengj
Copy link
Member Author

Oh, I guess this doesn't make sense in the case of a matrix log, since log(0.0) is -Inf, i.e. you shouldn't be taking the log of a semidefinite matrix anyway.

Note that this can change the output of sqrt by O(√ε), but it is still backwards stable. That is, B = sqrt(A) is a matrix such that ‖B^2 - A‖ / ‖A‖ is O(ε).

@stevengj stevengj removed needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Mar 10, 2020
Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

LGTM except for the test tolerances.

stdlib/LinearAlgebra/test/symmetric.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member Author

stevengj commented May 7, 2020

@andreasnoack, I updated the test tolerances some time ago and tests are passing. Good to merge?

@andreasnoack andreasnoack merged commit 0c388fc into master May 7, 2020
@andreasnoack andreasnoack deleted the stevengj-patch-5 branch May 7, 2020 20:12
eschnett pushed a commit to eschnett/julia that referenced this pull request May 11, 2020
* treat small negative λ as 0 for sqrt(::Hermitian) and log(::Hermitian)

* typo

* added tests, docs; removed rtol argument for log

* don't ask for rtol so close to eps(Float64)
Roger-luo pushed a commit to Roger-luo/julia that referenced this pull request May 14, 2020
* treat small negative λ as 0 for sqrt(::Hermitian) and log(::Hermitian)

* typo

* added tests, docs; removed rtol argument for log

* don't ask for rtol so close to eps(Float64)
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.

2 participants