Improve numerical symmetry check by CNZHM666 · Pull Request #1248 · python-control/python-control

CNZHM666 · 2026-09-13T10:01:33Z

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.

coveralls · 2026-09-15T00:38:04Z

coverage: 94.757%. remained the same — CNZHM666:fix-symmetry-check into python-control:main

slivingston · 2026-09-15T00:41:14Z

@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.)

slivingston · 2026-09-15T04:01:36Z

 from scipy.linalg import eigvals, solve

-from control.mateqn import lyap, dlyap, care, dare
+from control.mateqn import lyap, dlyap, care, dare,  _is_symmetric


Suggested change

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/

ilayn · 2026-09-15T04:40:10Z

SciPy already has these https://docs.scipy.org/doc/scipy/reference/generated/scipy.linalg.ishermitian.html

slivingston · 2026-09-15T05:39:06Z

@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)

CNZHM666 · 2026-09-15T10:01:37Z

@slivingston Sure, I’ll review those comments and update the implementation accordingly.

slivingston · 2026-09-15T20:28:16Z

+    return (
+        sp.linalg.norm(M - M.conj().T, 1)
+        <= np.spacing(sp.linalg.norm(M, 1)) * 100
+    )


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).

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?

@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.

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?

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.

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.

murrayrm

Please simplify the implementation by avoiding the use of **kwargs and just using symmetric_kwargs={} in the function definitions. This will simplify the code and let you remove the kwargs_test.py changes as well.

murrayrm · 2026-09-20T16:59:58Z



-def dlyap(A, Q, C=None, E=None, method=None):
+def dlyap(A, Q, C=None, E=None, method=None, **kwargs):


Is there a reason to implement this using **kwargs rather than just using symmetric_kwargs={} in the function signature. The latter would remove the need to add kwargs tests, both here and in tests/.

murrayrm · 2026-09-20T17:00:24Z


 def care(A, B, Q, R=None, S=None, E=None, stabilizing=True, method=None,
-         _As="A", _Bs="B", _Qs="Q", _Rs="R", _Ss="S", _Es="E"):
+         _As="A", _Bs="B", _Qs="Q", _Rs="R", _Ss="S", _Es="E", **kwargs):


As above, I suggest implementing as symmetric_kwargs={}.

murrayrm · 2026-09-20T17:00:49Z

+
+    symmetric_kwargs = kwargs.pop("symmetric_kwargs", {})
+    # Make sure there were no extraneous keywords
+    if kwargs:
+        raise TypeError("unrecognized keyword(s): ", str(kwargs))


All of this code could be removed by using symmetric_kwargs={} in the function definition.

murrayrm · 2026-09-20T17:01:32Z

+
+    symmetric_kwargs = kwargs.pop("symmetric_kwargs", {})
+
+    # Make sure there were no extraneous keywords
+    if kwargs:
+        raise TypeError("unrecognized keyword(s): ", str(kwargs))
+


Can be removed.

murrayrm · 2026-09-20T17:01:46Z


 def dare(A, B, Q, R, S=None, E=None, stabilizing=True, method=None,
-         _As="A", _Bs="B", _Qs="Q", _Rs="R", _Ss="S", _Es="E"):
+         _As="A", _Bs="B", _Qs="Q", _Rs="R", _Ss="S", _Es="E", **kwargs):


As above, use symmetric_kwargs={}.

murrayrm · 2026-09-20T17:01:55Z

+
+    symmetric_kwargs = kwargs.pop("symmetric_kwargs", {})
+
+    # Make sure there were no extraneous keywords
+    if kwargs:
+        raise TypeError("unrecognized keyword(s): ", str(kwargs))
+


murrayrm · 2026-09-20T17:03:40Z

+    symmetric_kwargs = (
+        symmetric_kwargs.copy() if symmetric_kwargs else {}
+    )
+


Is this required? I think you can just pass **symmetric_kwargs to ishermetian and issymmetric.

murrayrm · 2026-09-20T17:05:08Z

+    symmetric_kwargs = kwargs.pop('symmetric_kwargs', {})
+


OK to keep since kwargs was already present, but you could also just use symmetric_kwargs={} in the function definition (before **kwargs).

murrayrm · 2026-09-20T17:05:40Z

+    symmetric_kwargs = kwargs.pop('symmetric_kwargs', {})
+


As above, you could replace this with symmetric_kwargs={} in the function definition.

@murrayrm Updated as suggested: symmetric_kwargs is now an explicit parameter, and the related kwargs_test.py changes have been removed. All relevant tests and lint checks pass. Since test_mutable_defaults rejects {} as a default value, I used None and convert it to an empty dict only when needed.

murrayrm · 2026-09-24T02:41:16Z

@slivingston Are you OK with this version?

slivingston · 2026-09-24T04:35:00Z

        'slycot' and 'scipy'.  If set to None (default), try 'slycot' first
        and then 'scipy'.
+    symmetric_kwargs : dict, optional
+        Keyword arguments passed to the SciPy symmetry/Hermitian check,


My comment about the other docstring applies here, too.

slivingston · 2026-09-24T04:37:06Z

-            # Solve the Lyapunov equation using SciPy
-            return sp.linalg.solve_continuous_lyapunov(A, -Q)
+                # Solve the Lyapunov equation using SciPy
+                return sp.linalg.solve_continuous_lyapunov(A, -Q)


Why is this indented again? Given all of the whitespace errors in your changes, I recommend you check your editor settings and read PEP 8.

slivingston · 2026-09-24T04:38:58Z

@slivingston Are you OK with this version?

@murrayrm I think this parameter name is good, but there were documentation and whitespace errors that I just requested to change. If you are OK with it, after those are fixed, I will merge this.

CNZHM666 · 2026-09-24T06:23:40Z

@slivingston Updated the docstrings to explicitly name scipy.linalg.issymmetric and scipy.linalg.ishermitian, and cleaned up the unnecessary blank lines and whitespace changes. The relevant checks pass.

murrayrm

Looks OK except for one unaddressed comment from @slivingston. OK to merge once @slivingston signs off.

CNZHM666 · 2026-09-24T21:43:55Z

@murrayrm Fixed the remaining indentation issue and cleaned up the requested documentation/whitespace changes. The latest changes have been pushed.

slivingston

@CNZHM666 There are still problems with whitespace. Please take some time before requesting review to check this. It is trivial, yet it is repeatedly a problem with your code. Be more careful.

slivingston · 2026-09-29T06:34:24Z

@CNZHM666 Can you enable code changes to this PR from project maintainers (like me)? If I can just fix the whitespace problems on my own, then we can merge this simple PR.

https://docs.github.com/en/pull-requests/how-tos/work-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

CNZHM666 · 2026-09-29T06:51:01Z

@slivingston I already have “Allow edits from maintainers” enabled for this PR. Please feel free to fix the remaining whitespace issues directly. Thanks for your time and for helping me improve this PR.

python-control#1174

slivingston · 2026-09-29T07:23:02Z

@CNZHM666 Thanks for contributing!

slivingston self-requested a review September 15, 2026 00:21

slivingston requested changes Sep 15, 2026

View reviewed changes

Comment thread control/mateqn.py

Comment thread control/tests/mateqn_test.py

slivingston reviewed Sep 15, 2026

View reviewed changes

slivingston requested changes Sep 15, 2026

View reviewed changes

CNZHM666 requested a review from slivingston September 18, 2026 07:02

murrayrm requested changes Sep 20, 2026

View reviewed changes

CNZHM666 requested a review from murrayrm September 24, 2026 01:17

slivingston requested changes Sep 24, 2026

View reviewed changes

CNZHM666 requested a review from slivingston September 24, 2026 06:23

murrayrm reviewed Sep 24, 2026

View reviewed changes

Comment thread control/mateqn.py Outdated

CNZHM666 requested a review from murrayrm September 25, 2026 00:29

slivingston requested changes Sep 29, 2026

View reviewed changes

Comment thread control/tests/stochsys_test.py

Comment thread control/tests/stochsys_test.py

Comment thread control/tests/stochsys_test.py

use symmetry check from SciPy

067cf78

python-control#1174

slivingston force-pushed the fix-symmetry-check branch from 6f79ea2 to 067cf78 Compare September 29, 2026 07:21

slivingston merged commit b4cf736 into python-control:main Sep 29, 2026

slivingston mentioned this pull request Sep 29, 2026

better symmetry check test #1174

Closed

	from control.mateqn import lyap, dlyap, care, dare, _is_symmetric
	from control.mateqn import lyap, dlyap, care, dare, _is_symmetric



		def dlyap(A, Q, C=None, E=None, method=None):
		def dlyap(A, Q, C=None, E=None, method=None, **kwargs):

Conversation

CNZHM666 commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slivingston commented Sep 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilayn commented Sep 15, 2026

Uh oh!

slivingston commented Sep 15, 2026

Uh oh!

CNZHM666 commented Sep 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CNZHM666 Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murrayrm commented Sep 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slivingston commented Sep 24, 2026

Uh oh!

CNZHM666 commented Sep 24, 2026

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CNZHM666 commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CNZHM666 commented Sep 13, 2026 •

edited

Loading

coveralls commented Sep 15, 2026 •

edited

Loading

CNZHM666 Sep 21, 2026 •

edited

Loading

CNZHM666 commented Sep 24, 2026 •

edited

Loading