Fix 19547 is positive semidefinite incorrect #19573
Conversation
…7-is-positive-semidefinite-incorrect
|
✅ Hi, I am the SymPy bot (v160). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7. Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
sympy/matrices/tests/test_eigen.py
Outdated
| @@ -493,13 +498,19 @@ def test_definite(): | |||
| m = Matrix([[2, -1, 0], [-1, 2, -1], [0, -1, 2]]) | |||
| assert m.is_positive_definite == True | |||
| assert m.is_positive_semidefinite == True | |||
| assert _is_positive_semidefinite_by_cholesky_factorization_with_pivots(m) | |||
| assert _is_positive_semidefinite_by_eigenvalues(m) | |||
| assert _is_positive_semidefinite_by_minors(m) | |||
There was a problem hiding this comment.
There is a trailing whitespace at the end of line 503 which is causing quality tests to fail, please fix that.
There was a problem hiding this comment.
My bad there. I removed whitespace and updated the flake8 part of setup.cfg to catch it earlier next time.
There was a problem hiding this comment.
I guess adding whitespace to setup.cfg was a bad idea, since it caused a whole bunch of linting errors in the pipeline. Removed in latest commit.
sympy/matrices/eigen.py
Outdated
| return all(eigenvalue.is_nonnegative for eigenvalue in M.eigenvals()) | ||
|
|
||
|
|
||
| def _is_positive_semidefinite_by_cholesky_factorization_with_pivots(M): |
There was a problem hiding this comment.
These are too long names that can easily exceed 80 characters.
What about making them shorter like _is_positive_semidefinite_cholesky.
I don't think that it should need much mathematical meaning, especially if these functions are only used internaly.
There was a problem hiding this comment.
I agree they are a bit verbose. I tend to err on the side of more specificity in my variable names and comments. I shortened them.
…7-is-positive-semidefinite-incorrect
|
I think that going through all the tests can be too exhaustive. If cholesky can’t decide about symbolic values, I have doubts whether eigenvalues or sylvester is going to be more reliable for that. |
I thought using fallback methods would be the most robust approach, but I'm okay with just having it use Cholesky if you think using eigenvalues and Sylvester won't help much with the failing cases. |
Codecov Report
@@ Coverage Diff @@
## master #19573 +/- ##
=============================================
+ Coverage 75.663% 75.673% +0.010%
=============================================
Files 654 654
Lines 169932 169941 +9
Branches 40062 40066 +4
=============================================
+ Hits 128576 128601 +25
+ Misses 35739 35724 -15
+ Partials 5617 5616 -1 |
This PR adds more efficient algorithms for determining if a matrix is symmetric positive semidefinite than Sylvester's criterion. The first method tried is Cholesky factorization with complete pivoting as described here. If that fails, an eigenvalue criterion is used. If that also fails, Sylvester's criterion is used as a last resort.
References to other Issues or PRs
Related issue
Follow up to this PR
Brief description of what is fixed or changed
Adds two new private functions to
eigen.py_is_positive_semidefinite_cholesky_is_positive_semidefinite_evalsAlso changes the logic of
_is_positive_semidefiniteto attempt Cholesky factorization first, then eigenvalue method, then the original approach.Adds tests for the individual positive semidefinite test cases to help ensure all the methods are correct.
Other comments
Release Notes