Move _tf_close_coeff back to testing realm and make better use of assertion messages #1109

bnavigator · 2025-02-01T09:40:23Z

The OS/BLAS testing matrix started to fail with some macOS tests and the assertion messages make them hard to inspect:

https://github.com/python-control/python-control/actions/runs/13087050463/job/36519541162#step:9:298

 FAILED control/tests/statesp_test.py::TestStateSpace::test_pow - assert False
 +  where False = _tf_close_coeff(TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2), TransferFunction(\n[[array([1.]), array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2))
 +    where TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = minreal()
 +      where minreal = TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2).minreal
 +        where TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = ss2tf(StateSpace(\narray([], shape=(0, 0), dtype=float64),\narray([], shape=(0, 2), dtype=float64),\narray([], shape=(2, 0), dtype=float64),\narray([[1., 0.],\n       [0., 1.]]),\nstates=0, outputs=2, inputs=2))
 +    and   TransferFunction(\n[[array([1.]), array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = minreal()
 +      where minreal = TransferFunction(\n[[array([  1.,   3.,  -9., -41.]), \n  array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n ....,   3.,  -9., -41.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1., 0., 0., 0.])]],\noutputs=2, inputs=2).minreal
 +        where TransferFunction(\n[[array([  1.,   3.,  -9., -41.]), \n  array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n ....,   3.,  -9., -41.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1., 0., 0., 0.])]],\noutputs=2, inputs=2) = ss2tf(StateSpace(\narray([[  4.86563613,  -0.22114886, -14.38268526],\n       [  3.86388385,  -1.64824725, -16.2652334 ],\n    ... [0.00000000e+00, 2.59786829e-16, 3.85014366e-15]]),\narray([[1., 0.],\n       [0., 1.]]),\nstates=3, outputs=2, inputs=2))

Let's try to make this easier by using numpy's and pytest's assertion rewriting.

coveralls · 2025-02-01T09:56:55Z

coverage: 94.701% (+0.04%) from 94.659%
when pulling 2359299 on bnavigator:assert_tf_close_coeff
into 92bd703 on python-control:main.

bnavigator · 2025-02-01T10:28:34Z

The failures are happening in tests from #1081. Any thoughts @sdahdah?

bnavigator · 2025-02-01T10:43:05Z

Before:

___________________________ TestStateSpace.test_pow ____________________________

self = <control.tests.statesp_test.TestStateSpace object at 0x108ea1580>
sys222 = StateSpace(
array([[ 4.,  1.],
       [ 2., -3.]]),
array([[ 5.,  2.],
       [-3., -3.]]),
array([[ 2., -4.],
       [ 0.,  1.]]),
array([[ 3.,  2.],
       [ 1., -1.]]),
states=2, outputs=2, inputs=2)
sys322 = StateSpace(
array([[-3.,  4.,  2.],
       [-1., -3.,  0.],
       [ 2.,  5.,  3.]]),
array([[ 1.,  4.],
       [-3., ..., -3.],
       [ 1.,  4.,  3.]]),
array([[-2.,  4.],
       [ 0.,  1.]]),
name='sys322', states=3, outputs=2, inputs=2)

    @slycotonly
    def test_pow(self, sys222, sys322):
        """Test state space powers."""
        for sys in [sys222, sys322]:
            # Power of 0
            result = sys**0
            expected = StateSpace([], [], [], np.eye(2), dt=0)
            np.testing.assert_allclose(expected.A, result.A)
            np.testing.assert_allclose(expected.B, result.B)
            np.testing.assert_allclose(expected.C, result.C)
            np.testing.assert_allclose(expected.D, result.D)
            # Power of 1
            result = sys**1
            expected = sys
            np.testing.assert_allclose(expected.A, result.A)
            np.testing.assert_allclose(expected.B, result.B)
            np.testing.assert_allclose(expected.C, result.C)
            np.testing.assert_allclose(expected.D, result.D)
            # Power of -1 (inverse of biproper system)
            # Testing transfer function representations to avoid the
            # non-uniqueness of the state-space representation. Once MIMO
            # canonical forms are supported, can check canonical state-space
            # matrices instead.
            result = (sys * sys**-1).minreal()
            expected = StateSpace([], [], [], np.eye(2), dt=0)
            assert _tf_close_coeff(
                ss2tf(expected).minreal(),
                ss2tf(result).minreal(),
            )
            result = (sys**-1 * sys).minreal()
            expected = StateSpace([], [], [], np.eye(2), dt=0)
>           assert _tf_close_coeff(
                ss2tf(expected).minreal(),
                ss2tf(result).minreal(),
            )
E           assert False
E            +  where False = _tf_close_coeff(TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2), TransferFunction(\n[[array([1.]), array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2))
E            +    where TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = minreal()
E            +      where minreal = TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2).minreal
E            +        where TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = ss2tf(StateSpace(\narray([], shape=(0, 0), dtype=float64),\narray([], shape=(0, 2), dtype=float64),\narray([], shape=(2, 0), dtype=float64),\narray([[1., 0.],\n       [0., 1.]]),\nstates=0, outputs=2, inputs=2))
E            +    and   TransferFunction(\n[[array([1.]), array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = minreal()
E            +      where minreal = TransferFunction(\n[[array([  1.,   3.,  -9., -41.]), \n  array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n ....,   3.,  -9., -41.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1., 0., 0., 0.])]],\noutputs=2, inputs=2).minreal
E            +        where TransferFunction(\n[[array([  1.,   3.,  -9., -41.]), \n  array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n ....,   3.,  -9., -41.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1., 0., 0., 0.])]],\noutputs=2, inputs=2) = ss2tf(StateSpace(\narray([[  4.86563613,  -0.22114886, -14.38268526],\n       [  3.86388385,  -1.64824725, -16.2652334 ],\n    ... [0.00000000e+00, 2.59786829e-16, 3.85014366e-15]]),\narray([[1., 0.],\n       [0., 1.]]),\nstates=3, outputs=2, inputs=2))

control/tests/statesp_test.py:596: AssertionError

After:

________________ TestStateSpace.test_pow_inv[sys322-inv*sys] __________________

self = <control.tests.statesp_test.TestStateSpace object at 0x10b398260>
request = <FixtureRequest for <Function test_pow_inv[sys322-inv*sys]>>
sysname = 'sys322', order = 'inv*sys'

    @slycotonly
    @pytest.mark.parametrize("order", ["inv*sys", "sys*inv"])
    @pytest.mark.parametrize("sysname", ["sys222", "sys322"])
    def test_pow_inv(self, request, sysname, order):
        """Power of -1 (inverse of biproper system).
    
        Testing transfer function representations to avoid the
        non-uniqueness of the state-space representation. Once MIMO
        canonical forms are supported, can check canonical state-space
        matrices instead.
        """
        sys = request.getfixturevalue(sysname)
        if order == "inv*sys":
            result = (sys**-1 * sys).minreal()
        else:
            result = (sys * sys**-1).minreal()
        expected = StateSpace([], [], [], np.eye(2), dt=0)
>       assert_tf_close_coeff(
            ss2tf(expected).minreal(),
            ss2tf(result).minreal())

control/tests/statesp_test.py:601: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

tf_a = TransferFunction(
[[array([1.]), array([0.])],
 [array([0.]), array([1.])]],
[[array([1.]), array([1.])],
 [array([1.]), array([1.])]],
outputs=2, inputs=2)
tf_b = TransferFunction(
[[array([1.]), array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],
 [array([0.]), array([1.])]],
[[array([1.]), array([  1.,   3.,  -9., -41.])],
 [array([1.]), array([1.])]],
outputs=2, inputs=2)
rtol = 1e-05, atol = 1e-08

    def assert_tf_close_coeff(tf_a, tf_b, rtol=1e-5, atol=1e-8):
        """Check if two transfer functions have close coefficients.
    
        Parameters
        ----------
        tf_a : TransferFunction
            First transfer function.
        tf_b : TransferFunction
            Second transfer function.
        rtol : float
            Relative tolerance for ``np.testing.assert_allclose``.
        atol : float
            Absolute tolerance for ``np.testing.assert_allclose``.
    
        Raises
        ------
        AssertionError
        """
        # Check number of outputs and inputs
        assert tf_a.noutputs == tf_b.noutputs
        assert tf_a.ninputs == tf_b.ninputs
        # Check timestep
        assert  tf_a.dt == tf_b.dt
        # Check coefficient arrays
        for i in range(tf_a.noutputs):
            for j in range(tf_a.ninputs):
>               np.testing.assert_allclose(
                    tf_a.num[i][j],
                    tf_b.num[i][j],
                    rtol=rtol, atol=atol)
E               AssertionError: 
E               Not equal to tolerance rtol=1e-05, atol=1e-08
E               
E               (shapes (1,), (3,) mismatch)
E                ACTUAL: array([0.])
E                DESIRED: array([-2.763953e-15, -1.355413e-13,  9.015765e-13])

sdahdah · 2025-02-01T12:53:18Z

In these tests, I'm trying to test sys**-1. The only way I can think to do this is to check that sys**-1 * sys = sys * sys**-1 = 1. I compare the transfer function representations because they should be unique. It looks like minreal() is not removing the tiny poles in some cases, and this appears to be platform-dependent.

I'm starting to think that this test might just be flawed and should be removed entirely. Though I'm not sure what the best way to test sys**-1 would be. Numerically check frequency response over a range maybe?

Thanks for making assert_tf_close_coeff() more readable. One comment is that assert_allclose(actual, desired) has actual and desired as parameters, so assert_tf_close_coeff() should probably as well, instead of just a and b. It looks like the order is swapped. So the output should be:

E               (shapes (1,), (3,) mismatch)
E                ACTUAL: array([-2.763953e-15, -1.355413e-13,  9.015765e-13])
E                DESIRED: array([0.])

Sorry about the flaky unit tests 🙃

murrayrm · 2025-02-01T17:39:26Z

I came across this same issue at some point. The issues is that there are some very small numbers that pop up in the sys * sys**-1 on MacOS with OpenBLAS. If I remember correctly, this caused the numerator polynomial to have coefficients on order 1e-12.

I thought I fixed this someplace and the documentation PR (#1094) doesn't have this problem. But I can't find the fix I used in that PR. I'll keep looking.

…ertion messages

bnavigator · 2025-02-01T17:51:17Z

How about: 2359299?

bnavigator · 2025-02-01T17:53:23Z

I thought I fixed this someplace and the documentation PR (#1094) doesn't have this problem. But I can't find the fix I used in that PR. I'll keep looking.

#1094 (comment)

murrayrm

Looks good overall. Some minor comments about import style, but since this is test code it doesn't matter so much.

murrayrm · 2025-02-01T22:22:54Z

control/tests/xferfcn_test.py

+from control import (StateSpace, TransferFunction, defaults, evalfr, isctime,
+                     isdtime, reset_defaults, rss, sample_system, set_defaults,
+                     ss, ss2tf, tf, tf2ss, zpk)


Nonstandard. Use hanging indent.

murrayrm · 2025-02-01T22:23:03Z

control/tests/statesp_test.py

+from control.statesp import (StateSpace, _convert_to_statespace, _rss_generate,
+                             _statesp_defaults, drss, linfnorm, rss, ss, tf2ss)
+from control.tests.conftest import (assert_tf_close_coeff, editsdefaults,
+                                    slycotonly)


The default style for imports is to use "hanging indent" (isort -m2).

bnavigator force-pushed the assert_tf_close_coeff branch from 24af4fd to ac5bdd9 Compare February 1, 2025 10:51

bnavigator added 5 commits February 1, 2025 18:48

Move _tf_close_coeff back to testing realm and make better use of ass…

e69fc1a

…ertion messages

Fix docstring and imports

1ceeeea

Parametrize pow tests

1b6e49b

Rename tested tf to actual and desired

72aef92

Update test to check for inverse by identity

2359299

bnavigator force-pushed the assert_tf_close_coeff branch from 13a262d to 2359299 Compare February 1, 2025 17:50

bnavigator mentioned this pull request Feb 1, 2025

Updated documentation #1094

Merged

6 tasks

murrayrm approved these changes Feb 1, 2025

View reviewed changes

murrayrm merged commit 5707dcf into python-control:main Feb 3, 2025
49 checks passed

murrayrm added this to the 0.10.2 milestone Feb 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move _tf_close_coeff back to testing realm and make better use of assertion messages #1109

Move _tf_close_coeff back to testing realm and make better use of assertion messages #1109

Uh oh!

bnavigator commented Feb 1, 2025

Uh oh!

coveralls commented Feb 1, 2025 •

edited

Loading

Uh oh!

bnavigator commented Feb 1, 2025

Uh oh!

bnavigator commented Feb 1, 2025 •

edited

Loading

Uh oh!

sdahdah commented Feb 1, 2025

Uh oh!

murrayrm commented Feb 1, 2025

Uh oh!

bnavigator commented Feb 1, 2025

Uh oh!

bnavigator commented Feb 1, 2025 •

edited

Loading

Uh oh!

murrayrm left a comment

Uh oh!

murrayrm Feb 1, 2025

Uh oh!

murrayrm Feb 1, 2025

Uh oh!

Uh oh!

Uh oh!

Move _tf_close_coeff back to testing realm and make better use of assertion messages #1109

Move _tf_close_coeff back to testing realm and make better use of assertion messages #1109

Uh oh!

Conversation

bnavigator commented Feb 1, 2025

Uh oh!

coveralls commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnavigator commented Feb 1, 2025

Uh oh!

bnavigator commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdahdah commented Feb 1, 2025

Uh oh!

murrayrm commented Feb 1, 2025

Uh oh!

bnavigator commented Feb 1, 2025

Uh oh!

bnavigator commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

murrayrm Feb 1, 2025

Choose a reason for hiding this comment

Uh oh!

murrayrm Feb 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coveralls commented Feb 1, 2025 •

edited

Loading

bnavigator commented Feb 1, 2025 •

edited

Loading

bnavigator commented Feb 1, 2025 •

edited

Loading