Conversation
|
|
||
| # Utility function to check if a matrix is symmetric | ||
| def _is_symmetric(M): | ||
|
|
There was a problem hiding this comment.
Do not put a blank line after the start of a function def. Also, why did you add a line after M = np.atleast_2d(M) ? If this was an accident, please undo it.
| cdare(A, B, Qfs, R, S, E) | ||
| with pytest.raises(ControlArgument): | ||
| cdare(A, B, Q, Rfs, S, E) | ||
| def test_is_symmetric_scale_aware(self): |
There was a problem hiding this comment.
Can you improve the whitespace of your changes?
- Always at least 1 blank line before the start of a function def.
- The blank line between each
Mdefinition and respectiveassertstatement below does not improve clarity.
There was a problem hiding this comment.
Thanks for pointing that out. I have cleaned up the whitespace and rerun the relevant tests.
|
coverage: 94.756% (-0.001%) from 94.757% — CNZHM666:fix-symmetry-check into python-control:main |
|
@CNZHM666 Can you verify that your GitHub account is associated with the email address in your commit? (Until this is done, the avatar next to the commit appears generic in this PR.) |
| from scipy.linalg import eigvals, solve | ||
|
|
||
| from control.mateqn import lyap, dlyap, care, dare | ||
| from control.mateqn import lyap, dlyap, care, dare, _is_symmetric |
There was a problem hiding this comment.
| from control.mateqn import lyap, dlyap, care, dare, _is_symmetric | |
| from control.mateqn import lyap, dlyap, care, dare, _is_symmetric |
I have not yet started a technical review... but I continue to find whitespace/style problems. Please read https://peps.python.org/pep-0008/
|
SciPy already has these https://docs.scipy.org/doc/scipy/reference/generated/scipy.linalg.ishermitian.html |
|
@ilayn thanks for the link! I read through issue #1174, and indeed, the agreed solution is to use the method from SciPy. @CNZHM666 Can you do so? In particular, read #1174 (comment) and #1174 (comment) |
|
@slivingston Sure, I’ll review those comments and update the implementation accordingly. |
4 participants
This PR addresses #1174.
The current symmetry check does not properly handle complex Hermitian matrices. It also uses a fixed floating-point tolerance. Since floating-point rounding error depends on the numerical scale of the matrix, using a fixed tolerance can be too strict for matrices with large values.
I changed the check to use the conjugate transpose (
M.conj().T) and a scale-aware tolerance based on the matrix norm and floating-point spacing.I added tests for large-scale floating-point matrices, clearly asymmetric matrices, and complex Hermitian matrices.
AI disclosure:
I used ChatGPT to help me understand the numerical formulas involved in this issue and to assist with parts of the code changes and tests. I reviewed the changes myself, ran the tests locally, and understand the submitted code.