Link to issue: gh-2912

Related issues: gh-2973, gh-3259

Mailing list discussion: http://thread.gmane.org/gmane.comp.python.scientific.devel/19427

@befelix this PR now has a merge conflict, can you rebase it?

Seems mostly backwards compatible. I found one issue so far, related to the setters and the removal of _update():

>>> l = signal.lti([2.], [3.], [5], [6])
>>> l.C = 1   # used to work, now you get a hard to understand error
...
AttributeError: 'int' object has no attribute 'shape'

Is the added copy method useful? It doesn't do anything that can't be achieved by:

import copy
copy.copy(other_instance)

as far as I can tell.

I've rebased the code, should make it easier to review.
I'm a bit busy this week, but I'll try to get through all your recommendations by next weekend.

The regression with l.C seems very weird, especially since that's something I've definitely tested before making the PR. I'll have a look!

Just for completeness here are your comments from the mailing list:
"""

• I'd like the classes to have more Pythonic names, instead of too short names that are taken from Matlan
• The conversion functions should probably be named "to_zpk, to_ss, "to_tf" rather than "zpk, tf, ss".
• There's no SOS class, we do have that format now (ENH: Add second-order sections filtering #3717)
• Warren suggested "lti.from_tf/zpk/ss/sos" constructors on issue 3259. Looks like that should fit with your PR, but good to think about now.
• I think that this fits with where we want to go with input to filter functions (Filter design functions should accept other input than TF. #3259), but ideally we'd work that out together with this PR (not the implementation, but at least a sketch of the new API).

"""

I agree about the pythonic names, would make code using these classes super readable. Sos wasn't around when I wrote the PR, I'll check it out.

Hmm, SOS was merged in January so was in the code already, but I guess it hadn't made its way into http://docs.scipy.org/doc/scipy-dev/reference/signal.html by then. Double checked that it's included now.

Not sure what the best names would be by the way. StateSpace, TransferFunction, ZerosPolesGain? Especially the latter isn't very nice.

Or SsSystem, ZpkSystem, TfSystem?

The copy method is mostly there to support the setters for objects from different lti subclasses in a central location (i.e. the lti master class), rather than cluttering the subclasses with all the unrelated getters and setters (the code gets a bit ugly otherwise).
Consider the example of

obj = ss(1,1,1,1)
obj.poles = 1

What happens in the code is that the setter for poles in the lti class converts our object to the tf class tf_obj = obj.tf(), changes the poles, and the converts it back obj2 = tf_obj.ss(). The problem is that we now have a new object (obj2) that is different from obj, but should replace it. I don't think copy can handle that (a method of an object can't change the object itself, I think). So what the copy method in the subclasses does it copy all the relevant attributes of obj2 to obj.

This is the best solution I came up with, though on second thought copy() should probably a private method. Any ideas how to make this prettier are appreciated. If you would prefer to have all the setters/getters in the subclasses then it's possible to remove the copy function.

Regression with l.C = 1 is fixed. Thanks!

Regarding the names: I can't think of a very pretty solution either. Maybe ss/tf/zpk is not that bad after all :/.
Also changing ss to SteadyState seems a bit weird if lti is not renamed to LinearTimeInvariant.

I prefer capital letters for class names, whether it's SS(...) or Zpk(...) or so. I assume there isn't a scipy-wide standard...?

Sorry I haven't been following this. So overall, this will still behave like an LTI class that stores whatever coefficients you initialized it with, and can output every other form when asked?

@Eric89GXL Yeah I'm confused about the standard as well. Officially it's capital letters for classes I think, but then that doesn't really seem to be followed. The original lti class was already lower case, so at least in the short term there's probably nothing we can do about that.

@endolith The original LTI class had attributes for all systems (ss,zpk,tf). Changing one of the elements caused an _update() function to be called, which updated all other atrributes. This seemed a bit messy, so now there's seperate classes for it. However the old workflow/api still works. The difference is that with this PR you don't actually get an object of the lti class anymore. Calling lti(..) automatically returns an object of the appropriate subclass (ss, tf, zpk). So from the outside everything behaves like the old lti class, but inside it's all new and improved.

The standard is simply PEP8, however in the past it hasn't always been followed (Scipy itself is as old as the first draft of PEP8). That's especially true for scipy.signal, which is the only module with an API that has been largely modeled on Matlab API - something we want to get away from.

So proposal: let's follow PEP8 here, no matter whether we choose ZerosPolesGain, ZpkSystem, Zpk or yet something else.

+1 for following PEP8 from me, might as well start moving in that direction

I agree that PEP8 would be great. My question would be how far we can go with that given backwards compatibility and wether it makes sense to fix only parts of those.:

• lti is lower case
• the system matrices are upper case
• most functions in the ltisys.py file take upper case kwargs

If we can't change those maybe it's better to use the wrong style, but consistently?

@rgommers Regarding the SOS class. There is some initial code in https://github.com/befelix/scipy/tree/sos_class to add them to to the lti class. However, this causes a bunch of problems, since there is no conversion function sos<-->ss (for example impulse and step rely on this for default response times). Is there a reason for that? Is it 'okay' to just call something like sos_sys.to_zpk().to_ss()? Maybe this functionality and the according discussion should be in a separate PR?

So as far as I can tell the PR should be okay now code-wise, leaving only the questions of the SOS system and the API (i.e. naming and what should the inputs to ss/tf/zpk be: Currently it's just *args. Maybe it should be more like proposed in #3259, where they take one input, which can be a touple of parameters or an lti system?

Regarding the naming of variables I'm not sure and some input would be very welcome. Options seem to be

• StateSpace / TransferFunction / ZerosPolesGain / SecondOrderSections (like Maple)
• StateSpaceSystem / TransferFunctionSystem / ZerosPolesGainSystem / SecondOrderSectionsSystem or Model (Mathematica)
• Ss / Tf / Zpk / Sos (Matlab/Octave)
• SsSystem / TfSystem / ZpkSystem / SosSystem

I'm not very keen on the last one; mostly since SsSystem looks weird to me. I think the first one is probably the best balance of readability and length. Given that these functions are in scipy.signal it should be clear that those are models. In any case, what also needs to be decided if the likes of obj.to_tf() should then be renamed to obj.to_transfer_function() as well.

Regarding the StateSpace system matrices, we could maybe have both capital- (for backwards compatiblity) and lower case letters (for consistency)?

One last thought: I think in the long-run it would make sense to depreciate the cross-subclasses setters (e.g. setting the poles on a StateSpace system). They encourage extremely bad behaviour, require code that is not nice, and are, imho, not very useful.

SteadyState / TransferFunction / ZerosPolesGain / SecondOrderSections (like Maple)

Shouldn't that be StateSpace?

Yes, yes it should be :)

+1 for Ss / Tf / Zpk / Sos from me -- it seems quick and easy. Long names make my fingers tired and my PEP8 code more wrappy. My +0.5 for the maple versions. But I'm really okay with any of those.

I think it should be okay to have sos_sys.to_zpk().to_ss() going on under the hood if one does sos_sys.to_ss(), at least for a first pass. In my head sos is only one small step away from a zpk representation, where that step is chosen to be easily implemented as an IIR filter that minimizes numerical problems. @endolith WDYT?

+1 for a single input argument instead of *args. It seems to lend itself to easier interpretation and implementation in my head, but I haven't thought about it extensively.

For the names, how are these intended to be used? If you're only ever going to create objects using a generic Lti or Filter name, and it returns one of these classes, does it matter that the names are long? Explicit names are generally more clear than abbreviations.

What I'm imagining for "easy filtering" is like this:

my_lpf = butter(5, 0.2, output='Filter')
output = my_lpf(input)

where input and output are ndarrays, and it would automatically generate the filter using zpk coefficients, return my_lpf, which is an instance of some class that internally stores the zpk coefficients, then the instance is callable, which automatically creates an SOS and filters the input signal using sosfilt(). The filter instance could also output any other representation, like my_lpf.to_ss(), calculate frequency response like my_lpf.freqresp(w), etc.

In my head sos is only one small step away from a zpk representation

I'm not sure, I was reading about this but don't fully understand it: http://www.bisonacademy.com/ECE461/Lectures/06%20State%20Space.pdf Controller and observer canonical forms are made from the tf coefficients, Jordan form is made from the partial fraction expansion coefficients (like scipy.signal.residuez?), and cascade form is similar in concept to sos but uses the zpk coefficients along the diagonal? But maybe it's only for all-pole systems?

Also remember that SS supports MIMO systems and the others don't, etc.

Alright, that should wrap up all the of @Eric89GXL's comments. Is there anything else that needs to change?
Also I rebased the code on master because of a merge conflict with the recent lsim rewrite. I hope that's okay.

@befelix rebasing is our preferred way of fixing merge conflict, so this looks good.

I just quickly browsed over the code again, and quite like the look of this API now.

Is there any hope of getting this merged before the 0.16 branching?

@befelix I think it's possible, but I'm kind of running out of time. @Eric89GXL can you verify that all your comments are addressed and this is okay to merge for you? Then I'm happy to merge this after a quick last sanity check.

Other than my minor complains, LGTM.

Alright, thanks for the last comments, they're all implemented.
@Eric89GXL: *args are not actually handled consistently in scipy, but I think your *system: arguments suggestion is good.

Okay, in it goes. Thanks @befelix!

And thanks @Eric89GXL and @endolith for reviewing.

Thanks again everyone. In case you feel like looking through even more subclasses, there is a follow-up PR at #4881, which extends this framework to discrete-time systems. It would be great if you could give comments on that as well!