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.764% (+0.007%) 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. |
| return ( | ||
| sp.linalg.norm(M - M.conj().T, 1) | ||
| <= np.spacing(sp.linalg.norm(M, 1)) * 100 | ||
| ) |
There was a problem hiding this comment.
Where is the "SciPy symmetry check method" in this commit?
A few quick questions:
- Can you read the comments that I linked to previously, which describe the desired changes, without using an AI bot?
- Can you try to write the change without using an AI bot?
Using AI tools is OK, but I want to make sure that you understand the proposed solution (and thus, can write/understand the code).
There was a problem hiding this comment.
Yes. I read the linked comments and SciPy documentation myself.
I misunderstood the requested change earlier. I now understand that I should use SciPy’s built-in symmetry/Hermitian check rather than reimplementing the norm/spacing test. I will rewrite the change myself.
Should the implementation use SciPy’s default exact comparison, or should atol/rtol also be passed through?
There was a problem hiding this comment.
@CNZHM666 I recommend that atol/rtol are passed through. Let me highlight text from one of the comments that I linked:
A couple of thoughts on things we might do:
- We should almost certainly replace _issymmetric with scipy.issymmetric, since there is no reason for the duplication.
- We could add a way to allow rtol and atol to be passed through to scipy.issymmetric, so that the user can control the behavior better. There are several other examples where we pass down options to scipy functions.
- We might also include an option to symmetrize either Q or QN (via (M + M.T)*0.5, as @ilayn suggests).
Whoever picks up this issue should look through the code and see what makes the most sense.
There was a problem hiding this comment.
I traced the call chain. lqe/dlqe already accept keyword arguments and call care/dare, while care, dare, lyap, and dlyap currently do not expose symmetry tolerances. Do you want atol/rtol to be added to all of these public matrix-equation functions, or only passed through the LQE path for this issue?
There was a problem hiding this comment.
Let's try to use kwargs or a similar name to support passing through the parameter to scipy. (Probably better not to add explict atol, rtol parameters.) As in the above comment, "There are several other examples where we pass down options to scipy functions." Find those examples, and try to follow the pattern in this PR.
There was a problem hiding this comment.
I followed your suggestion by replacing the original symmetry check with SciPy's built-in symmetry/Hermitian checks. I also used a symmetric_kwargs pass-through so parameters such as atol and rtol can be passed from the upper-level APIs down to SciPy.I also added tests to verify that a nearly symmetric matrix fails with the default exact check but succeeds when an appropriate tolerance is passed through.
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.