- Notifications
You must be signed in to change notification settings - Fork441
Allow np.array or np.matrix for state space matrices, operations#314
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 commentedJun 15, 2019 • 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.
coveralls commentedJun 15, 2019
control/bdalg.py Outdated
| >>> 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 |
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?
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.
| eye(self.inputs)) | ||
| eye(self.inputs) | ||
| + np.dot(other.fresp[:, :, k], self.fresp[:, :, k]), | ||
| eye(self.inputs)) |
roryyorkeJun 16, 2019 • 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.
I know this is not the point of this change, but islinalg(X, eye(shape)) any better thannp.linalg.inv? I'd have thought a "right divide" (linalg.solve with appropriate transposes) would have been a more standard approach.
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 usesp.linalg.solve instead, but since I wasn't the original author I just did the conversion to remove thenp.matrix dependence. I've added a TODO for someone to fix this, so at least we don't forget about it (in principle).
control/mateqn.py Outdated
| @@ -41,16 +41,17 @@ | |||
| Author: Bjorn Olofsson | |||
| """ | |||
| from scipy import shape, size,asarray, asmatrix, copy, zeros, eye, dot | |||
| from scipy import shape, size,array, asarray, copy, zeros, eye, dot | |||
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 removeimport scipy as sp, I think this should befrom numpy import ...
| @@ -69,7 +70,9 @@ def lyap(A,Q,C=None,E=None): | |||
| :math:`A X E^T + E X A^T + Q = 0` | |||
| where Q is a symmetric matrix and A, Q and E are square matrices | |||
| of the same dimension. """ | |||
| of the same dimension. | |||
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.
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 anumpydoc standard:https://numpydoc.readthedocs.io/en/latest/format.html#docstring-standard
| @@ -252,13 +253,13 @@ def acker(A, B, poles): | |||
| # 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) | |||
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 thematrix_power call sets the stage for the upcoming for-loop.
| 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)) |
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?
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.
CI tests are failing due to issue#321. Need to fix before merging. |
This PR addresses issue#233 regarding (eventual) deprecation of the
numpy.matrixclass. In this PR, state space objects are stored as eithernumpy.matrix(default) ornumpy.arrayobjects, with the selection determined based on theuse_numpy_array()function.For backward compatibility, the
numpy.matrixclass is the current default, but all operations are compatible with usingnumpy.arrayas the default storage and output class and this will be the default whennumpy.matrixis deprecated.A set of unit tests is also included for testing basic operations when the
numpy.arrayclass is used as the state space storage and output class. This PR does not convert all of the existing unit tests to usenumpy.arrayinstead ofnumpy.matrix, so there are still lots of warning messages. At some point we will need to update all unit tests to work withnumpy.arrayobjects (as we as done in abandoned PR#292).