fixes to unit tests so that they pass when the default array type is ndarray #433

sawyerbfuller · 2020-07-22T20:46:08Z

instead of deprecated matrix class. Changes entailed:

slicing fixes in code. In particular, if A is a matrix, A[:, 1] is still a 2D matrix, but if A is an ndarray, that slice results in a 1D vector. To fix, this was changed to e.g. A[:, 1:2], which perserves that the array is 2D after slicing.
use np.dot instead of * to multiply matrices in a few unit tests
if B or C are given as 1d arrays, new code in statesp.__init__ attempts to determine whether they should be row or column vectors.
completes tasks in Allow np.array or np.matrix for state space matrices, operations #314

I checked on my computer and unit tests also pass when the default array type is matrix.

…y instead of matrix. two failing unit tests to fix still

coveralls · 2020-07-23T09:19:35Z

Coverage increased (+0.01%) to 84.222% when pulling ebd73c1 on sawyerbfuller:ndarray-is-default into d3142ff on python-control:master.

bnavigator · 2020-07-23T21:13:05Z

I made a test merge in bnavigator#1 and reactivated TravisCI for my fork. Tests look good so far, there were merge conflicts in both merges, because the edits were in lines next to each other. Please check, if I resolved correctly.

bnavigator · 2020-07-23T21:19:33Z

Regarding the actual PR: Could you add a CI job that runs the tests with default ndarray too?

bnavigator · 2020-07-23T21:27:47Z

control/statesp.py

-    'statesp.use_numpy_matrix': True,
+    'statesp.use_numpy_matrix': False,


Shouldn't we only change defaults in a major release?

How about keeping the default, but adding a CI job as proposed to maintain ndarray compatibility until the default is changed for good? At the time of the default change, keep a CI job that tests for compatibility with matrix as long as it is supported.

indeed. I don't know what the release plans are for big changes (wasn't sure this would go into the next release), but let me push a commit that reverses this for now, but that way we can still have unit test fixes.

sawyerbfuller · 2020-07-23T22:07:55Z

I made a test merge in bnavigator#1 and reactivated TravisCI for my fork. Tests look good so far, there were merge conflicts in both merges, because the edits were in lines next to each other. Please check, if I resolved correctly.

Looks good to me -- I think!

sawyerbfuller · 2020-07-23T22:14:38Z

Regarding the actual PR: Could you add a CI job that runs the tests with default ndarray too?

Could you possibly point me to where I might configure that? runtests.py?

bnavigator · 2020-07-23T22:56:53Z

I was a about to create a PR into yours, but your repo does not show up, when I try to select the Base Repository? Please pull the commit yourself: bnavigator@eda91ca

git remote add bnavigator https://github.com/bnavigator/python-control.git
git fetch --all
git merge bnavigator/pr433

sawyerbfuller · 2020-07-23T23:18:33Z

Ok sure. Kids just got home so I will have to do it when I have a moment at the computer later this evening.

On Thu, Jul 23, 2020 at 3:57 PM Ben Greiner ***@***.***> wrote: I was a about to create a PR into yours, but your repo does not show up, when I try to select the Base Repository? Please pull the commit yourself: ***@***.*** <bnavigator@eda91ca> git remote add bnavigator https://github.com/bnavigator/python-control.git git fetch --all git merge bnavigator/pr433 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#433 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN74SSO3KI2ZUSKN2O4BWCTR5C54HANCNFSM4PFCUGCA> .

-- Sawyer Fuller Assistant Professor of Mechanical Engineering University of Washington http://faculty.washington.edu/~minster/ (Typed with my thumbs)

bnavigator · 2020-07-23T23:26:28Z

... and it found three more matrix/ndarray mismatches:
https://travis-ci.org/github/bnavigator/python-control/jobs/711275561

=================================== FAILURES ===================================
1967________________________ TestCanonical.test_modal_form _________________________
1968
1969self = <control.tests.canonical_test.TestCanonical testMethod=test_modal_form>
1970
1971    def test_modal_form(self):
1972        """Test the modal canonical form"""
1973    
1974        # Create a system in the modal canonical form
1975        A_true = np.diag([4.0, 3.0, 2.0, 1.0]) # order from the largest to the smallest
1976        B_true = np.matrix("1.1 2.2 3.3 4.4").T
1977        C_true = np.matrix("1.3 1.4 1.5 1.6")
1978        D_true = 42.0
...
2016>       sys_check, T_check = canonical_form(ss(A, B, C, D), 'modal')
2017
2018control/tests/canonical_test.py:107: 
2019_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2020control/canonical.py:42: in canonical_form
2021    return modal_form(xsys)
2022control/canonical.py:211: in modal_form
2023    zsys.A = solve(Tzx, xsys.A).dot(Tzx)
2024<__array_function__ internals>:5: in solve
2025    ???
2026../../../miniconda/envs/test-environment/lib/python3.8/site-packages/numpy/linalg/linalg.py:385: in solve
2027    _assert_stacked_2d(a)
2028_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2029
2030arrays = (array([-0.56303537, -0.3564182 ,  0.06171638, -0.22836009,  0.        ,
2031        0.17111809, -0.54651494, -0.41477707,  0.40688007, -0.8141637 ,
2032       -0.38002113,  0.16483334, -0.44769516,  0.15654653, -0.50060858,
2033        0.72419146]),)
2034a = array([-0.56303537, -0.3564182 ,  0.06171638, -0.22836009,  0.        ,
2035        0.17111809, -0.54651494, -0.41477707,  0.40688007, -0.8141637 ,
2036       -0.38002113,  0.16483334, -0.44769516,  0.15654653, -0.50060858,
2037        0.72419146])
2038
2039    def _assert_stacked_2d(*arrays):
2040        for a in arrays:
2041            if a.ndim < 2:
2042>               raise LinAlgError('%d-dimensional array given. Array must be '
2043                        'at least two-dimensional' % a.ndim)
2044E               numpy.linalg.LinAlgError: 1-dimensional array given. Array must be at least two-dimensional
2045
2046../../../miniconda/envs/test-environment/lib/python3.8/site-packages/numpy/linalg/linalg.py:206: LinAlgError

Matrix to ndarray conversion produced the wrong dimension.


2047______________________ TestCanonical.test_observable_form ______________________
2048
2049self = <control.tests.canonical_test.TestCanonical testMethod=test_observable_form>
2050
2051    def test_observable_form(self):
2052        """Test the observable canonical form"""
2053    
...
2058        B_true = np.matrix("1.0 1.0 1.0 1.0").T
...
2071    
2072        # Create a state space system and convert it to the observable canonical form
2073        sys_check, T_check = canonical_form(ss(A, B, C, D), "observable")
2074    
2075        # Check against the true values
2076        np.testing.assert_array_almost_equal(sys_check.A, A_true)
2077>       np.testing.assert_array_almost_equal(sys_check.B, B_true)
2078E       AssertionError: 
2079E       Arrays are not almost equal to 6 decimals
2080E       
2081E       (shapes (4, 4), (4, 1) mismatch)
2082E        x: array([[ 0.508834,  0.748576, -1.417827, -0.827351],
2083E              [-0.134752, -0.070451, -0.032659, -0.090651],
2084E              [-0.18486 ,  0.369904,  0.172657, -0.07489 ],
2085E              [-0.222568,  0.077826, -0.248874,  0.360027]])
2086E        y: matrix([[1.],
2087E               [1.],
2088E               [1.],
2089E               [1.]])
2090
2091control/tests/canonical_test.py:190: AssertionError

sys_check.B does not look right. constructor error or canonical_form?

2092________________________ TestCanonical.test_similarity _________________________
2093
2094self = <control.tests.canonical_test.TestCanonical testMethod=test_similarity>
2095
2096    def test_similarity(self):
2097        """Test similarty transform"""
2098    
2099        # Single input, single output systems
2100        siso_ini = tf2ss(tf([1, 1], [1, 1, 1]))
2101        for form in 'reachable', 'observable':
2102            # Convert the system to one of the canonical forms
2103            siso_can, T_can = canonical_form(siso_ini, form)
2104    
2105            # Use a similarity transformation to transform it back
2106>           siso_sim = similarity_transform(siso_can, np.linalg.inv(T_can))
2107
2108control/tests/canonical_test.py:231: 
2109_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2110control/canonical.py:238: in similarity_transform
2111    zsys = StateSpace(xsys)
2112_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2113
2114self = StateSpace(array([[-1.,  1.],
2115       [-1.,  0.]]), array([[ 1., -1.],
2116       [ 0., -0.]]), array([[1., 0.]]), array([[0.]]))
2117args = (StateSpace(array([[-1.,  1.],
2118       [-1.,  0.]]), array([[ 1., -1.],
2119       [ 0., -0.]]), array([[1., 0.]]), array([[0.]])),)
2120kw = {}, A = array([[-1.,  1.],
2121       [-1.,  0.]])
2122B = array([[ 1., -1.],
2123       [ 0., -0.]]), C = array([[1., 0.]])
2124D = array([[0.]]), dt = None, remove_useless = True
2125
2126    def __init__(self, *args, **kw):
2127        """
2128        StateSpace(A, B, C, D[, dt])
...
2206>           raise ValueError("B and D must have the same number of columns.")
2207E           ValueError: B and D must have the same number of columns.
2208
2209control/statesp.py:246: ValueError

Same as above. canonical_form returns invalid StateSpace or similarity_transform does something weird.

sawyerbfuller · 2020-07-24T02:47:50Z

I was a about to create a PR into yours, but your repo does not show up, when I try to select the Base Repository? Please pull the commit yourself: bnavigator@eda91ca
git remote add bnavigator https://github.com/bnavigator/python-control.git
git fetch --all
git merge bnavigator/pr433

Should I do a git push upstream now that it's merged?

sawyerbfuller · 2020-07-24T02:48:34Z

As for the errors you're seeing, I don't see them. To run tests locally, I've been running python setup.py test but maybe that's not running everything? Thanks.

bnavigator · 2020-07-24T10:08:18Z

As for the errors you're seeing, I don't see them. To run tests locally, I've been running python setup.py test but maybe that's not running everything? Thanks.

That one is deprecated.
Use pytest -v nowadays. Only pytest picks up the conftest.py
We should adjust the Readme and setup.py settings. #380 has been merged quite a while ago.

Should I do a git push upstream now that it's merged?

Not upstream but into your origin. I saw you did. And you see the failure in Travis CI here, too.

sawyerbfuller · 2020-07-24T17:45:43Z

OK I think I got those three bugs squashed. waiting on Travis to confirm.

bnavigator · 2020-07-24T18:12:47Z

BTW, I am working on the overall matrix/array testing. https://github.com/bnavigator/python-control/tree/array-matrix-tests

bnavigator · 2020-08-17T15:42:26Z

In my opinion, this PR should go into 0.8.4. The patch release will still have matrix as default output, but when setting to array manually things would break without this fix.

murrayrm · 2020-08-17T15:44:46Z

Since this is mainly a developer-facing change (right?), seems OK to push it into master sooner rather than later (and hence have it be part of 0.8.4).

bnavigator · 2020-08-17T15:54:10Z

The user facing changes are in statesp.py:

slicing fixes in code. In particular, if A is a matrix, A[:, 1] is still a 2D matrix, but if A is an ndarray, that slice results in a 1D vector. To fix, this was changed to e.g. A[:, 1:2], which perserves that the array is 2D after slicing.

if B or C are given as 1d arrays, new code in statesp.__init__ attempts to determine whether they should be row or column vectors.

Now a user can use arrays.

murrayrm · 2020-08-17T15:56:20Z

Sounds backwards compatible, so seems fine for 0.8.4.

sawyerbfuller added 2 commits July 22, 2020 12:55

initial commit changing default array type in statespace to be ndarra…

7e7ae9c

…y instead of matrix. two failing unit tests to fix still

found two remaining unit test bugs! (switch from * to np.dot)

5ecc543

sawyerbfuller changed the title ~~change default array type in statespace to be ndarrray~~ fixes to unit tests so that they pass when the default array type is ndarray Jul 23, 2020

bnavigator mentioned this pull request Jul 23, 2020

Mergetest Prs #400 #431 #433 bnavigator/python-control#1

Closed

7 tasks

bnavigator reviewed Jul 23, 2020

View reviewed changes

stick with old default for now

606fa3f

bnavigator mentioned this pull request Jul 23, 2020

Default dt is 0 #431

Closed

add conftest.py fixture and travis job to check with ndarray

eda91ca

bnavigator mentioned this pull request Jul 24, 2020

Readme still mentions setup.py test and setup.py requires nose instead of pytest #436

Closed

fixes in canonical and canonical_test that were assuming numpy.matrix

ebd73c1

bnavigator mentioned this pull request Jul 25, 2020

Refactor the test suite using pytest for array and matrix types #438

Merged

bnavigator added this to the 0.8.4 milestone Aug 17, 2020

bnavigator merged commit 07a7c7a into python-control:master Aug 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes to unit tests so that they pass when the default array type is ndarray #433

fixes to unit tests so that they pass when the default array type is ndarray #433

sawyerbfuller commented Jul 22, 2020 •

edited

Loading

coveralls commented Jul 23, 2020 •

edited

Loading

bnavigator commented Jul 23, 2020

bnavigator commented Jul 23, 2020

bnavigator Jul 23, 2020 •

edited

Loading

sawyerbfuller Jul 23, 2020

sawyerbfuller commented Jul 23, 2020

sawyerbfuller commented Jul 23, 2020

bnavigator commented Jul 23, 2020

sawyerbfuller commented Jul 23, 2020 via email

bnavigator commented Jul 23, 2020

sawyerbfuller commented Jul 24, 2020

sawyerbfuller commented Jul 24, 2020

bnavigator commented Jul 24, 2020 •

edited

Loading

sawyerbfuller commented Jul 24, 2020

bnavigator commented Jul 24, 2020

bnavigator commented Aug 17, 2020

murrayrm commented Aug 17, 2020

bnavigator commented Aug 17, 2020

murrayrm commented Aug 17, 2020

		'statesp.use_numpy_matrix': True,
		'statesp.use_numpy_matrix': False,

fixes to unit tests so that they pass when the default array type is ndarray #433

fixes to unit tests so that they pass when the default array type is ndarray #433

Conversation

sawyerbfuller commented Jul 22, 2020 • edited Loading

coveralls commented Jul 23, 2020 • edited Loading

bnavigator commented Jul 23, 2020

bnavigator commented Jul 23, 2020

bnavigator Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

sawyerbfuller Jul 23, 2020

Choose a reason for hiding this comment

sawyerbfuller commented Jul 23, 2020

sawyerbfuller commented Jul 23, 2020

bnavigator commented Jul 23, 2020

sawyerbfuller commented Jul 23, 2020 via email

bnavigator commented Jul 23, 2020

sawyerbfuller commented Jul 24, 2020

sawyerbfuller commented Jul 24, 2020

bnavigator commented Jul 24, 2020 • edited Loading

sawyerbfuller commented Jul 24, 2020

bnavigator commented Jul 24, 2020

bnavigator commented Aug 17, 2020

murrayrm commented Aug 17, 2020

bnavigator commented Aug 17, 2020

murrayrm commented Aug 17, 2020

sawyerbfuller commented Jul 22, 2020 •

edited

Loading

coveralls commented Jul 23, 2020 •

edited

Loading

bnavigator Jul 23, 2020 •

edited

Loading

bnavigator commented Jul 24, 2020 •

edited

Loading