-
Notifications
You must be signed in to change notification settings - Fork 438
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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)) |
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.
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).
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 remove import scipy as sp
, I think this should be from 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 a numpydoc
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 the matrix_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. |
…hon-control#314) * creates a new function `config.use_numpy_matrix()` that allows either `np.array` or `np.matrix` for state space matrices + unit tests * allow D=0 to broadcast to correct size for MIMO statespace systems
…bug (python-control#365) PR python-control#314 duplicates a lot of code in the test cases by introducing *_array_test.py files. Thus issue python-control#190 addressed in PR python-control#320 resurfaces and needs to be introduced to robust_array_test.pyas well.
This PR addresses issue #233 regarding (eventual) deprecation of the
numpy.matrix
class. In this PR, state space objects are stored as eithernumpy.matrix
(default) ornumpy.array
objects, with the selection determined based on theuse_numpy_array()
function.For backward compatibility, the
numpy.matrix
class is the current default, but all operations are compatible with usingnumpy.array
as the default storage and output class and this will be the default whennumpy.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 usenumpy.array
instead ofnumpy.matrix
, so there are still lots of warning messages. At some point we will need to update all unit tests to work withnumpy.array
objects (as we as done in abandoned PR #292).