GeezerVison

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

GeezerVison is showing github.com — Allow np.array or np.matrix for state space matrices, operations by murrayrm · Pull Request #314 · python-control/python-control

Conversation

murrayrm commented Jun 15, 2019

Copy link

Copy Markdown

Member

This PR addresses issue #233 regarding (eventual) deprecation of the numpy.matrix class. In this PR, state space objects are stored as either numpy.matrix (default) or numpy.array objects, with the selection determined based on the use_numpy_array() function.

For backward compatibility, the numpy.matrix class is the current default, but all operations are compatible with using numpy.array as the default storage and output class and this will be the default when numpy.matrix is deprecated.

A set of unit tests is also included for testing basic operations when the numpy.array class is used as the state space storage and output class. This PR does not convert all of the existing unit tests to use numpy.array instead of numpy.matrix, so there are still lots of warning messages. At some point we will need to update all unit tests to work with numpy.array objects (as we as done in abandoned PR #292).

Sorry, something went wrong.

Uh oh!

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

All reactions


          allow np.array or np.matrix for state space matrices + unit tests

0c1e983

murrayrm mentioned this pull request

Jun 15, 2019

Prepare for removal of dependency on numpy.matrix class #292

Closed

coveralls commented Jun 15, 2019 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Coverage increased (+0.06%) to 82.173% when pulling 7c9d9a6 on murrayrm:ss_matrix into e2346cd on python-control:master.

All reactions

Sorry, something went wrong.

Uh oh!

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

coveralls commented Jun 15, 2019

Copy link

Copy Markdown

Coverage increased (+0.07%) to 82.179% when pulling 0c1e983 on murrayrm:ss_matrix into e2346cd on python-control:master.

All reactions

Sorry, something went wrong.

Uh oh!

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

roryyorke reviewed

Jun 16, 2019

View reviewed changes

Comment thread

control/bdalg.py Outdated

		>>> sys2 = ss([[-1.]], [[1.]], [[1.]], [[0.]])
		>>> sys = append(sys1, sys2)
		>>> Q = sp.mat([ [ 1, 2], [2, -1] ]) # basically feedback, output 2 in 1
		>>> Q = [[1, 2], [2, -1]]) # negative feedback interconnection

roryyorke Jun 16, 2019

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.

trailing parenthesis here looks wrong. Do we run these with doctest?

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm Jun 17, 2019

Copy link

Copy Markdown

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

Fixed.

We are not currently using doctest, but I ran the revised version manually to make sure it was OK.

Sorry, something went wrong.

Uh oh!

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

All reactions

roryyorke reviewed

Jun 16, 2019

View reviewed changes

Comment thread

control/frdata.py

		eye(self.inputs))
		eye(self.inputs)
		+ np.dot(other.fresp[:, :, k], self.fresp[:, :, k]),
		eye(self.inputs))

roryyorke Jun 16, 2019 •

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.

I know this is not the point of this change, but is linalg(X, eye(shape)) any better than np.linalg.inv? I'd have thought a "right divide" (linalg.solve with appropriate transposes) would have been a more standard approach.

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm Jun 17, 2019

Copy link

Copy Markdown

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

I agree that this could use sp.linalg.solve instead, but since I wasn't the original author I just did the conversion to remove the np.matrix dependence. I've added a TODO for someone to fix this, so at least we don't forget about it (in principle).

Sorry, something went wrong.

Uh oh!

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

All reactions

roryyorke reviewed

Jun 16, 2019

View reviewed changes

Comment thread

control/mateqn.py Outdated

		"""

		from scipy import shape, size, asarray, asmatrix, copy, zeros, eye, dot
		from scipy import shape, size, array, asarray, copy, zeros, eye, dot

roryyorke Jun 16, 2019

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.

again not the point, but since you remove import scipy as sp, I think this should be from numpy import ...

Sorry, something went wrong.

Uh oh!

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

All reactions

roryyorke reviewed

Jun 16, 2019

View reviewed changes

Comment thread

control/mateqn.py

		where Q is a symmetric matrix and A, Q and E are square matrices
		of the same dimension. """
		of the same dimension.

roryyorke Jun 16, 2019

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.

Not super-familiar with PEP8 and so on, but this extra blank line seems odd.

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm Jun 17, 2019

Copy link

Copy Markdown

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

The extra blank line is a numpydoc standard: https://numpydoc.readthedocs.io/en/latest/format.html#docstring-standard

Sorry, something went wrong.

Uh oh!

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

All reactions

roryyorke reviewed

Jun 16, 2019

View reviewed changes

Comment thread

control/statefbk.py

		# Place the poles using Ackermann's method
		n = np.size(p)
		pmat = p[n-1]a*0
		pmat = p[n-1] * np.linalg.matrix_power(a, 0)

roryyorke Jun 16, 2019

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.

np.eye(a.shape) ? I suppose the matrix_power call sets the stage for the upcoming for-loop.

Sorry, something went wrong.

Uh oh!

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

All reactions

roryyorke reviewed

Jun 16, 2019

View reviewed changes

Comment thread

control/statefbk.py

		pmat = p[n-1] * np.linalg.matrix_power(a, 0)
		for i in np.arange(1,n):
		pmat = pmat + p[n-i-1]a*i
		pmat = pmat + np.dot(p[n-i-1], np.linalg.matrix_power(a, i))

roryyorke Jun 16, 2019

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.

Again somewhat beside the point of this commit, but would it be possible to use Horner's method here?

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm Jun 17, 2019

Copy link

Copy Markdown

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

Added a TODO for someone to do this in a separate PR.

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm added 2 commits

June 16, 2019 17:44


          fixed docstring example error + added TODO's in response to review

4c9c2c8


          allow D=0 to broadcast to correct size for MIMO statespace systems

4f48c5d

murrayrm mentioned this pull request

Jun 30, 2019

Decide on how to handle (to be deprecated?) np.matrix class #233

Closed


          change np.bmat -> np.block in timeresp.py

7c9d9a6

murrayrm commented Jun 30, 2019

Copy link

Copy Markdown

Member Author

CI tests are failing due to issue #321. Need to fix before merging.

All reactions

Sorry, something went wrong.

Uh oh!

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

murrayrm merged commit de9e3e7 into python-control:master

Jun 30, 2019

murrayrm deleted the ss_matrix branch

July 1, 2019 05:20

murrayrm added this to the 0.8.3 milestone

Jan 4, 2020

bnavigator mentioned this pull request

Jan 5, 2020

copy PR 320 for robust_array_test #365

Merged

murrayrm pushed a commit that referenced this pull request

Jan 5, 2020


          Copy PR #320 fix for robust_array_test to fix OpenSUSE bug (#365)

a7f5f1c

PR #314 duplicates a lot of code in the test cases by introducing *_array_test.py files. Thus issue #190 addressed in PR #320 resurfaces and needs to be introduced to robust_array_test.pyas well.

sawyerbfuller mentioned this pull request

Jul 23, 2020

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

Merged

bnavigator mentioned this pull request

Dec 2, 2021

vectorize FRD feedback function #680

Merged

Reviewers

1 more reviewer

roryyorke

roryyorke left review comments

Reviewers whose approvals may not affect merge requirements

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

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

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

Conversation

murrayrm commented Jun 15, 2019

Uh oh!

coveralls commented Jun 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryyorke Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murrayrm commented Jun 30, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jun 15, 2019 •

edited

Loading

Uh oh!

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

roryyorke Jun 16, 2019 •

edited

Loading

Uh oh!

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