GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fixes to unit tests so that they pass when the default array type is ndarray by sawyerbfuller · Pull Request #433 · python-control/python-control

Conversation

@sawyerbfuller

@sawyerbfuller sawyerbfuller commented Jul 22, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller added 2 commits July 22, 2020 12:55
@sawyerbfuller
initial commit changing default array type in statespace to be ndarra…
7e7ae9c 
…y instead of matrix. two failing unit tests to fix still
@sawyerbfuller
found two remaining unit test bugs! (switch from * to np.dot)
5ecc543
@coveralls

coveralls commented Jul 23, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller 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 bnavigator mentioned this pull request Jul 23, 2020
Mergetest Prs #400 #431 #433 bnavigator/python-control#1
Closed
7 tasks done
@bnavigator

bnavigator commented Jul 23, 2020

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

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

bnavigator commented Jul 23, 2020

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnavigator
bnavigator reviewed Jul 23, 2020
View reviewed changes
Comment thread control/statesp.py Outdated
# Define module default parameter values
_statesp_defaults = {
'statesp.use_numpy_matrix': True,
'statesp.use_numpy_matrix': False,

@bnavigator bnavigator Jul 23, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

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

@sawyerbfuller sawyerbfuller Jul 23, 2020

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sawyerbfuller
stick with old default for now
606fa3f
@sawyerbfuller

sawyerbfuller commented Jul 23, 2020

Copy link
Copy Markdown
Contributor Author

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!
All reactions

Sorry, something went wrong.

Uh oh!

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

sawyerbfuller commented Jul 23, 2020

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator bnavigator mentioned this pull request Jul 23, 2020
Default dt is 0 #431
Closed
@bnavigator
add conftest.py fixture and travis job to check with ndarray
eda91ca
@bnavigator

bnavigator commented Jul 23, 2020

Copy link
Copy Markdown
Contributor

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
All reactions

Sorry, something went wrong.

Uh oh!

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

sawyerbfuller commented Jul 23, 2020 via email

Copy link
Copy Markdown
Contributor Author
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 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

bnavigator commented Jul 23, 2020

Copy link
Copy Markdown
Contributor

... 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.
All reactions

Sorry, something went wrong.

Uh oh!

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

sawyerbfuller commented Jul 24, 2020

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

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

sawyerbfuller commented Jul 24, 2020

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

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

bnavigator commented Jul 24, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator bnavigator mentioned this pull request Jul 24, 2020
Readme still mentions setup.py test and setup.py requires nose instead of pytest #436
Closed
@sawyerbfuller
fixes in canonical and canonical_test that were assuming numpy.matrix
ebd73c1
@sawyerbfuller

sawyerbfuller commented Jul 24, 2020

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

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

bnavigator commented Jul 24, 2020

Copy link
Copy Markdown
Contributor

BTW, I am working on the overall matrix/array testing. https://github.com/bnavigator/python-control/tree/array-matrix-tests
sawyerbfuller 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 bnavigator mentioned this pull request Jul 25, 2020
Refactor the test suite using pytest for array and matrix types #438
Merged
@bnavigator

bnavigator commented Aug 17, 2020

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator bnavigator added this to the 0.8.4 milestone Aug 17, 2020
@murrayrm

murrayrm commented Aug 17, 2020

Copy link
Copy Markdown
Member

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 and sawyerbfuller reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

bnavigator commented Aug 17, 2020

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

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

murrayrm commented Aug 17, 2020

Copy link
Copy Markdown
Member

Sounds backwards compatible, so seems fine for 0.8.4.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator
bnavigator merged commit 07a7c7a into python-control:master Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnavigator bnavigator bnavigator left review comments

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.8.4

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

4 participants

@sawyerbfuller @coveralls @bnavigator @murrayrm
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.