Skip to content

statespace constructor fix #163

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

statespace constructor fix #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion control/canonical.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ def reachable_form(xsys):
"Canonical forms for MIMO systems not yet supported")

# Create a new system, starting with a copy of the old one
zsys = StateSpace(xsys)
print(xsys)
#print(*xsys)
l=[xsys.A,xsys.B,xsys.C,xsys.D,xsys.dt]
zsys = StateSpace(*l)

# Generate the system matrices for the desired canonical form
zsys.B = zeros(shape(xsys.B))
Expand Down
5 changes: 0 additions & 5 deletions control/setup.py
View file
Open in desktop

This file was deleted.

37 changes: 11 additions & 26 deletions control/statesp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,38 +91,21 @@ class StateSpace(LTI):

"""

def __init__(self, *args):
def __init__(self,A,B,C,D,dt=None):
Copy link

Choose a reason for hiding this comment

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

This change will break previous code where users made use of the key word arg, e.g. StateSpace(1, 2, 3, 4, dt=0.1) will fail with a TypeError.

Copy link
Author

Choose a reason for hiding this comment

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

I don't really know what you mean, you can still pass arguments as *args after the change, like this:

args=(A,B,C,D)
StateSpace(*args)
args=(A,B,C,D,dt)
StateSpace(*args)

The only thing the change will break is that you can no longer pass a StateSpace instance to StateSpace.__init__ to create a new one, you'll have to be explicit. I think the work required to unpack the values is a reasonable tradeoff to the simplicity gained.

Copy link

Choose a reason for hiding this comment

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

In [6]: def f(*args):
   ...:     return args
   ...: 

In [7]: f(1, 2, 3, 4, dt=0.5)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-7-ced6aff2bf09> in <module>()
----> 1 f(1, 2, 3, 4, dt=0.5)

TypeError: f() got an unexpected keyword argument 'dt'

Copy link
Author

Choose a reason for hiding this comment

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

Your code is an example of the old behavior, that I removed.

This is the new behavior:

>>> def f(a,b,c,d,dt=None):
...     return a,b,c,d,dt
...     
>>> f(1,2,3,4,dt=None)
(1, 2, 3, 4, None)

Copy link

Choose a reason for hiding this comment

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

You are right. Dyslexia I suppose :)

"""Construct a state space object.

The default constructor is StateSpace(A, B, C, D), where A, B, C, D are
matrices or equivalent objects. To call the copy constructor, call
StateSpace(sys), where sys is a StateSpace object.

Matrix sizes need to be consistent:
A must be square.
A and B must have the same number of rows.
A and C must have the same number of columns.
B and D must have the same number of columns.
C and D must have the same number of rows.

"""

if len(args) == 4:
# The user provided A, B, C, and D matrices.
(A, B, C, D) = args
dt = None;
elif len(args) == 5:
# Discrete time system
(A, B, C, D, dt) = args
elif len(args) == 1:
# Use the copy constructor.
if not isinstance(args[0], StateSpace):
Copy link

Choose a reason for hiding this comment

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

I suggest using if isinstance(A, StateSpace) to preserve this behavior for backwards compatibility.

Copy link
Author

Choose a reason for hiding this comment

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

The point of the changes in the pull request is to remove the ambiguity of being able to define statespaces in two ways:

S1=StateSpace(A,B,C,D)
S2=StateSpace(oldstatespace)

Yes that will break code for people who use the second method.

That probably should have been in the initial comment.

If this is breaking things too much for now, maybe it would be an option to create a "next major release" branch to include this change. That might make the transition obvious and it would make it easier to use the old version in the future.

If you don't want the module to be backwards incompatible at all, reject the pull request or tell me and I'll delete it.

Also your suggested change wouldn't work because the StateSpace.__init__ 

__init__(A,B,C,D,dt=None):

would expect B,C,D and throw an error if you only gave it a StateSpace instance as first argument.

The only way to preserve the functionality of being able to define a StateSpace with both, a previous instance or a list of variables, is to keep the current code.

raise TypeError("The one-argument constructor can only take in \
a StateSpace object. Recived %s." % type(args[0]))
A = args[0].A
B = args[0].B
C = args[0].C
D = args[0].D
try:
dt = args[0].dt
except NameError:
dt = None;
else:
raise ValueError("Needs 1 or 4 arguments; received %i." % len(args))

A, B, C, D = map(matrix, [A, B, C, D])

# TODO: use super here?
Expand All @@ -131,6 +114,7 @@ def __init__(self, *args):
self.B = B
self.C = C
self.D = D
self.dt=dt

self.states = A.shape[1]

Expand Down Expand Up @@ -491,7 +475,7 @@ def minreal(self, tol=0.0):
except ImportError:
raise TypeError("minreal requires slycot tb01pd")
else:
return StateSpace(self)
return StateSpace(self.A,self.B,self.C,self.D,self.dt)


# TODO: add discrete time check
Expand Down Expand Up @@ -545,6 +529,7 @@ def append(self, other):

def __getitem__(self, indices):
"""Array style acces"""

if len(indices) != 2:
raise IOError('must provide indices of length 2 for state space')
i = indices[0]
Expand Down
7 changes: 7 additions & 0 deletions control/tests/canonical_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,10 @@ def test_unreachable_system(self):

# Check if an exception is raised
np.testing.assert_raises(ValueError, canonical_form, sys, "reachable")

if __name__=="__main__":
t=TestCanonical()
t.test_reachable_form()
t.test_unreachable_system()
a=1
#unittest.main()
6 changes: 5 additions & 1 deletion control/tests/discrete_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,12 @@ def testSystemInitialization(self):

def testCopyConstructor(self):
for sys in (self.siso_ss1, self.siso_ss1c, self.siso_ss1d):
newsys = StateSpace(sys);

args=[sys.A,sys.B,sys.C,sys.D,sys.dt]
newsys = StateSpace(*args);

self.assertEqual(sys.dt, newsys.dt)

for sys in (self.siso_tf1, self.siso_tf1c, self.siso_tf1d):
newsys = TransferFunction(sys);
self.assertEqual(sys.dt, newsys.dt)
Expand Down