slicing fixes in code. In particular, ifA is amatrix,A[:, 1] is still a 2D matrix, but ifA is anndarray, 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 instatesp.__init__ attempts to determine whether they should be row or column vectors.
completes tasks inAllow 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 ismatrix.

sawyerbfuller added2 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

Copy link

coveralls commentedJul 23, 2020•
edited
Loading

Coverage increased (+0.01%) to 84.222% when pullingebd73c1 on sawyerbfuller:ndarray-is-default intod3142ff on python-control:master.

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 #433bnavigator/python-control#1

Closed

7 tasks

Copy link

Contributor

bnavigator commentedJul 23, 2020

I made a test merge inbnavigator#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.

Copy link

Contributor

bnavigator commentedJul 23, 2020

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

bnavigator reviewed

Jul 23, 2020

View reviewed changes

control/statesp.py Outdated

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

Copy link

Contributor

bnavigatorJul 23, 2020•
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

sawyerbfullerJul 23, 2020

Choose a reason for hiding this comment

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

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.

stick with old default for now

606fa3f

Copy link

ContributorAuthor

sawyerbfuller commentedJul 23, 2020

I made a test merge inbnavigator#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!

Copy link

ContributorAuthor

sawyerbfuller commentedJul 23, 2020

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

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

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

Copy link

Contributor

bnavigator commentedJul 23, 2020

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.gitgit fetch --allgit merge bnavigator/pr433

Copy link

ContributorAuthor

sawyerbfuller commentedJul 23, 2020 via email

Ok sure. Kids just got home so I will have to do it when I have a moment atthe 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 bnavigatorhttps://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 FullerAssistant Professor of Mechanical EngineeringUniversity of Washingtonhttp://faculty.washington.edu/~minster/(Typed with my thumbs)

Copy link

Contributor

bnavigator commentedJul 23, 2020

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

=================================== FAILURES ===================================1967________________________ TestCanonical.test_modal_form _________________________19681969self = <control.tests.canonical_test.TestCanonical testMethod=test_modal_form>19701971    def test_modal_form(self):1972        """Test the modal canonical form"""1973    1974        # Create a system in the modal canonical form1975        A_true = np.diag([4.0, 3.0, 2.0, 1.0]) # order from the largest to the smallest1976        B_true = np.matrix("1.1 2.2 3.3 4.4").T1977        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')20172018control/tests/canonical_test.py:107: 2019_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 2020control/canonical.py:42: in canonical_form2021    return modal_form(xsys)2022control/canonical.py:211: in modal_form2023    zsys.A = solve(Tzx, xsys.A).dot(Tzx)2024<__array_function__ internals>:5: in solve2025    ???2026../../../miniconda/envs/test-environment/lib/python3.8/site-packages/numpy/linalg/linalg.py:385: in solve2027    _assert_stacked_2d(a)2028_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 20292030arrays = (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])20382039    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-dimensional20452046../../../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 ______________________20482049self = <control.tests.canonical_test.TestCanonical testMethod=test_observable_form>20502051    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 form2073        sys_check, T_check = canonical_form(ss(A, B, C, D), "observable")2074    2075        # Check against the true values2076        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 decimals2080E       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.]])20902091control/tests/canonical_test.py:190: AssertionError

sys_check.B does not look right. constructor error orcanonical_form?

2092________________________ TestCanonical.test_similarity _________________________20932094self = <control.tests.canonical_test.TestCanonical testMethod=test_similarity>20952096    def test_similarity(self):2097        """Test similarty transform"""2098    2099        # Single input, single output systems2100        siso_ini = tf2ss(tf([1, 1], [1, 1, 1]))2101        for form in 'reachable', 'observable':2102            # Convert the system to one of the canonical forms2103            siso_can, T_can = canonical_form(siso_ini, form)2104    2105            # Use a similarity transformation to transform it back2106>           siso_sim = similarity_transform(siso_can, np.linalg.inv(T_can))21072108control/tests/canonical_test.py:231: 2109_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 2110control/canonical.py:238: in similarity_transform2111    zsys = StateSpace(xsys)2112_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 21132114self = 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 = True21252126    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.22082209control/statesp.py:246: ValueError

Same as above.canonical_form returns invalidStateSpace orsimilarity_transform does something weird.

Copy link

ContributorAuthor

sawyerbfuller commentedJul 24, 2020

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.gitgit fetch --allgit merge bnavigator/pr433

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

Copy link

ContributorAuthor

sawyerbfuller commentedJul 24, 2020

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

Copy link

Contributor

bnavigator commentedJul 24, 2020•
edited
Loading

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

That one is deprecated.
Usepytest -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.

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

Copy link

ContributorAuthor

sawyerbfuller commentedJul 24, 2020

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

Copy link

Contributor

bnavigator commentedJul 24, 2020

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

bnavigator mentioned this pull request

Jul 25, 2020

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

Merged

Copy link

Contributor

bnavigator commentedAug 17, 2020

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

bnavigator added this to the0.8.4 milestone

Aug 17, 2020

Copy link

Member

murrayrm commentedAug 17, 2020

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

Copy link

Contributor

bnavigator commentedAug 17, 2020

The user facing changes are instatesp.py:

slicing fixes in code. In particular, ifA is amatrix,A[:, 1] is still a 2D matrix, but ifA is anndarray, 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 instatesp.__init__ attempts to determine whether they should be row or column vectors.

Now a usercan use arrays.

Copy link

Member

murrayrm commentedAug 17, 2020

Sounds backwards compatible, so seems fine for 0.8.4.

bnavigator merged commit07a7c7a intopython-control:master

Aug 17, 2020

Labels

None yet

4 participants

Movatterモバイル変換

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

Uh oh!

Conversation

sawyerbfuller commentedJul 22, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

coveralls commentedJul 23, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

bnavigator commentedJul 23, 2020

Uh oh!

bnavigator commentedJul 23, 2020

Uh oh!

bnavigatorJul 23, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sawyerbfullerJul 23, 2020

Choose a reason for hiding this comment

Uh oh!

sawyerbfuller commentedJul 23, 2020

Uh oh!

sawyerbfuller commentedJul 23, 2020

Uh oh!

bnavigator commentedJul 23, 2020

Uh oh!

sawyerbfuller commentedJul 23, 2020 via email

Uh oh!

bnavigator commentedJul 23, 2020

Uh oh!

sawyerbfuller commentedJul 24, 2020

Uh oh!

sawyerbfuller commentedJul 24, 2020

Uh oh!

bnavigator commentedJul 24, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

sawyerbfuller commentedJul 24, 2020

Uh oh!

bnavigator commentedJul 24, 2020

Uh oh!

bnavigator commentedAug 17, 2020

Uh oh!

murrayrm commentedAug 17, 2020

Uh oh!

bnavigator commentedAug 17, 2020

Uh oh!

murrayrm commentedAug 17, 2020

Uh oh!

Uh oh!

sawyerbfuller commentedJul 22, 2020•
edited
Loading

coveralls commentedJul 23, 2020•
edited
Loading

bnavigatorJul 23, 2020•
edited
Loading

bnavigator commentedJul 24, 2020•
edited
Loading