GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Move _tf_close_coeff back to testing realm and make better use of assertion messages by bnavigator · Pull Request #1109 · python-control/python-control

Conversation

bnavigator commented Feb 1, 2025

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coveralls commented Feb 1, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator commented Feb 1, 2025

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator commented Feb 1, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator force-pushed the assert_tf_close_coeff branch from 24af4fd to ac5bdd9 Compare

February 1, 2025 10:51

sdahdah commented Feb 1, 2025

Copy link

Copy Markdown

Contributor

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 🙃

bnavigator reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm commented Feb 1, 2025

Copy link

Copy Markdown

Member

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.

sdahdah reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bnavigator force-pushed the assert_tf_close_coeff branch from 13a262d to 2359299 Compare

February 1, 2025 17:50

bnavigator commented Feb 1, 2025

Copy link

Copy Markdown

Contributor Author

How about: 2359299?

sdahdah reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator mentioned this pull request

Feb 1, 2025

Updated documentation #1094

Merged

6 tasks done

bnavigator commented Feb 1, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm approved these changes

Feb 1, 2025

View reviewed changes

murrayrm left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/tests/xferfcn_test.py

Comment on lines +13 to +15

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

murrayrm Feb 1, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nonstandard. Use hanging indent.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/tests/statesp_test.py

Comment on lines +22 to +25

		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)

murrayrm Feb 1, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm merged commit 5707dcf into python-control:main

Feb 3, 2025

murrayrm added this to the 0.10.2 milestone

Feb 19, 2025

Reviewers

murrayrm

murrayrm approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.10.2

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Feb 1, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

bnavigator commented Feb 1, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

bnavigator commented Feb 1, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.