- Notifications
You must be signed in to change notification settings - Fork441
Move _tf_close_coeff back to testing realm and make better use of assertion messages#1109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedFeb 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
bnavigator commentedFeb 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Before: ___________________________TestStateSpace.test_pow____________________________self=<control.tests.statesp_test.TestStateSpaceobjectat0x108ea1580>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)@slycotonlydeftest_pow(self,sys222,sys322):"""Test state space powers."""forsysin [sys222,sys322]:# Power of 0result=sys**0expected=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 1result=sys**1expected=sysnp.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(), )EassertFalseE+whereFalse=_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+whereTransferFunction(\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+whereminreal=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).minrealE+whereTransferFunction(\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+andTransferFunction(\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+whereminreal=TransferFunction(\n[[array([1.,3.,-9.,-41.]), \narray([-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).minrealE+whereTransferFunction(\n[[array([1.,3.,-9.,-41.]), \narray([-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.TestStateSpaceobjectat0x10b398260>request=<FixtureRequestfor<Functiontest_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"])deftest_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)iforder=="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-08defassert_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 inputsasserttf_a.noutputs==tf_b.noutputsasserttf_a.ninputs==tf_b.ninputs# Check timestepasserttf_a.dt==tf_b.dt# Check coefficient arraysforiinrange(tf_a.noutputs):forjinrange(tf_a.ninputs):>np.testing.assert_allclose(tf_a.num[i][j],tf_b.num[i][j],rtol=rtol,atol=atol)EAssertionError:ENotequaltotolerancertol=1e-05,atol=1e-08EE (shapes (1,), (3,)mismatch)EACTUAL:array([0.])EDESIRED:array([-2.763953e-15,-1.355413e-13,9.015765e-13]) |
24af4fd toac5bdd9CompareIn these tests, I'm trying to test 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 Thanks for making Sorry about the flaky unit tests 🙃 |
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. |
13a262d to2359299CompareHow about:2359299? |
bnavigator commentedFeb 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
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.
| from control import(StateSpace, TransferFunction, defaults, evalfr, isctime, | ||
| isdtime, reset_defaults, rss, sample_system, set_defaults, | ||
| ss, ss2tf, tf,tf2ss, zpk) |
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.
| 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) |
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).
5707dcf intopython-control:mainUh oh!
There was an error while loading.Please reload this page.
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
Let's try to make this easier by using numpy's and pytest's assertion rewriting.