Fix QRdecomposition for symbolic cases
#23316
Conversation
|
✅ Hi, I am the SymPy bot (v163). 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.11. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
d38ba55 to
e9aeeba
Compare
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) Full benchmark results can be found as artifacts in GitHub Actions |
| @@ -153,6 +155,18 @@ def test_QR(): | |||
| assert R.is_upper | |||
| assert A == Q*R | |||
|
|
|||
| x = Symbol('x') | |||
There was a problem hiding this comment.
probably should be x = Symbol('x', nonzero=True)? Otherwise, the Q matrix is not well-defined.
There was a problem hiding this comment.
...and would the original code have worked in that case? That suggests that a better fix may be possible.
There was a problem hiding this comment.
It does seem suspicious to be able to take a path through this GS/MGS and get empty (degenerate) matrices.
Here's another case to worry about:
Matrix([[0]]).QRdecomposition()
Out[42]: (Matrix(1, 0, []), Matrix(0, 1, []))
(That should give Q=[[1]] and R=[[0]].)
There was a problem hiding this comment.
Oh I see, this is a "thin" rank-revealing QR factorization.
>>> q, r = Matrix([[0]]).QRdecomposition()
>>>: q
Matrix(2, 0, [])
>>> r
Matrix(0, 2, [])
>>> q*r
Matrix([[0]])
I also tested with 2x2 zero matrix and it also works! That is pretty neat! At least some of this seems to be unit-tested too.
| x = Symbol('x') | ||
| A = Matrix([x]) | ||
| Q, R = A.QRdecomposition() | ||
| assert Q == Matrix([x / Abs(x)]) | ||
| assert R == Matrix([Abs(x)]) | ||
|
|
||
| A = Matrix([[x, 0], [0, x]]) | ||
| Q, R = A.QRdecomposition() | ||
| assert Q == x / Abs(x) * Matrix([[1, 0], [0, 1]]) | ||
| assert R == Abs(x) * Matrix([[1, 0], [0, 1]]) |
There was a problem hiding this comment.
I'd probably use a new def test_QR_with_symbols(): but don't feel strongly.
| @@ -1363,7 +1363,7 @@ def dot(u, v): | |||
| Q[:, j] -= Q[:, i] * R[i, j] | |||
|
|
|||
| Q[:, j] = dps(Q[:, j]) | |||
| if Q[:, j].is_zero_matrix is False: | |||
| if Q[:, j].is_zero_matrix is not True: | |||
There was a problem hiding this comment.
Ok, having experimented with the below, I agree with this change.
But perhaps it could use a comment:
# if we are sure the column is zero, we do not need to include it
if Q[:, j].is_zero_matrix in (False, None):
Or how about this without a comment?
if Q[:, j].is_zero_matrix:
continue
ranked.append(j)
R[j, j] = M.one
|
Looks good to me. |
References to other Issues or PRs
Fixes #23313
Brief description of what is fixed or changed
I'm attempting to fix the issue by changing the fuzzy logic to infer
is_zero == Noneas nonzero.Other comments
Release Notes
Matrix.QRdecompositiongiving wrong results for matrices containing symbols (for most cases where zero test can automatically infer the correct results)