statespace constructor fix #163
Conversation
…py wasn't used anywhere
| @@ -91,38 +91,21 @@ class StateSpace(LTI): | |||
|
|
|||
| """ | |||
|
|
|||
| def __init__(self, *args): | |||
| def __init__(self,A,B,C,D,dt=None): | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
You are right. Dyslexia I suppose :)
| (A, B, C, D, dt) = args | ||
| elif len(args) == 1: | ||
| # Use the copy constructor. | ||
| if not isinstance(args[0], StateSpace): |
There was a problem hiding this comment.
I suggest using if isinstance(A, StateSpace) to preserve this behavior for backwards compatibility.
There was a problem hiding this comment.
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.
|
The intent of allowing |
|
I can add a |
|
It seems to me that we are fixing a non-problem. I suggest we drop this pull request and leave the current functionality. |
|
You don't see a problem with |
|
I am not a core dev with python-control but use it often. My rule of thumb is to not break backwards compatibility unless absolutely necessary. The amount of code you may break and potential amount of frustrated users is almost always not worth the change. How about improving the documentation and examples of use for the class instead of changing the functionality? Then it is is a win win for everyone. |
|
Agree with @moorepants that a good approach is to update the documentation. I'll try to take a cut at that in the coming days and will close out the PR if issue is fully addressed. |
|
I have created a new PR (#171) that fixes the documentation for the class constructors. |
...other changes related, the deleted setup.py wasn't used anywhere as far as I could see.