By the way, numeric re-linearization of the system like this is convenient, but probably introduces inaccuracy. Probably the right way to do it in a future PR would be to correctly combine the constituent systems using their A, B, C, and D matrices using theconnect function.

Coverage decreased (-0.002%) to 94.765% when pulling98fba6e on sawyerbfuller:interconnect-ss intobeb629b on python-control:main.

I'm not sure this is the best way to fix this issue. If I understand it correctly, it essentially starts putting member functions from theInputOutputSystem class into theStateSpace class. But that is what theLinearIOSystem class is for => we are essentially replicating what is already there.

TheStateSpace class is intended to be a "low level" class that is typically not seen by the user (if you use thess factory function, you get anLinearIOSystem object, which inherits from bothInputOutputSystem andStateSpace). TheStateSpace class is mainly there for legacy purposes and because I thought there might be some speed advantages (though I'm not sure this is in fact the case).

I need to think a bit more about how else this might be done, perhaps also thinking through the role of theLinearICSystem class, as described in issue#786. Some alternatives would be to "eliminate" the StateSpace class completely (probably by just having it be another name forLinearIOSystem) or converting aStateSpace object to aLinearIOSystem object for any operation with anInputOutputSystem object.

Also, there are a lot of changes that are part of this PR that seem to be related to previous commits. I'm not sure if the changes are based on the main branch or not.

Also, there are a lot of changes that are part of this PR that seem to be related to previous commits. I'm not sure if the changes are based on the main branch or not.

Seems that a rebase/merge has gone wrong.

Fixed by force-pushed rebase.

@sawyerbfuller you need to reset your local branch before continuing to work on this.

I'm not sure this is the best way to fix this issue. If I understand it correctly, it essentially starts putting member functions from theInputOutputSystem class into theStateSpace class. But that is what theLinearIOSystem class is for => we are essentially replicating what is already there.
TheStateSpace class is intended to be a "low level" class that is typically not seen by the user (if you use thess factory function, you get anLinearIOSystem object, which inherits from bothInputOutputSystem andStateSpace). TheStateSpace class is mainly there for legacy purposes and because I thought there might be some speed advantages (though I'm not sure this is in fact the case).

I agree having duplicate methods doesn't make much sense. To that end, and following the idea thatStateSpace should be a low-level class, the recent commit leaves_rhs and_out inStateSpace but takes them out ofLinearIOSystem. Instead, references to the methods already inStateSpace are added in to to override the empty stubs that are defined inInputOutputSystem. This is consistent with the idea thatStateSpace be a low-level class -- maybe one that that contains numerical methods. Let me know what you think.

I need to think a bit more about how else this might be done, perhaps also thinking through the role of theLinearICSystem class, as described in issue#786. Some alternatives would be to "eliminate" the StateSpace class completely (probably by just having it be another name forLinearIOSystem) or converting aStateSpace object to aLinearIOSystem object for any operation with anInputOutputSystem object.

Might make sense to eventually mergeStateSpace andLinearIOSystem classes somehow.

Also, there are a lot of changes that are part of this PR that seem to be related to previous commits. I'm not sure if the changes are based on the main branch or not.

Fixed, I think. Syncing issues. Thanks@bnavigator

So I looked through this PR a bit more carefully. It looks to me like there are three different things going on:

1. Update StateSpace class so thatinterconnect will allowLinearIOSystem andStateSpace objects to be combined correctly.
2. Addparams as a keyword todynamics andoutput.
3. Update the way "static gain" is determined.

I have comments and questions for each, plus an alternative PR#788 that fixes the first two.

StateSpace updates

I think the issue being addressed here is that if you try to useinterconnect with a list of systems that includes a mixture ofLinearIOSystem objects andStateSpace objects, it doesn't work correctly, even though the operator overloading for those combinations handles things properly.

The proposed fix is to update theStateSpace class to have some of the methods that normally go with anInputOutputSystem object and then use those in theLinearIOSystem object. That will work, but it messes around with the class structure in a somewhat spaghetti like way and leads to some inconsistencies. For example, the fix does not allow you tointerconnect aTransferFunction with aLinearIOSystem, even though operator overloading on that case works. In principle we could add_rhs and_out to theTransferFunction class to fix that, but of course it really doesn't make sense for a transfer function to have a right-hand side. It's just that it needs to be converted to aLinearIOSystem object via some realization (which is what the operator overloading implementation does).

I think that a better short term fix is to update theinterconnect function so that it recognizesStateSpace andTransferFunction objects and properly converts them toLinearIOSystem objects. I did that in PR#788 by adding 4 lines of code (and some more extensive unit tests).

In the long term, we might want to combineStateSpace andLinearIOSystem into a single class, since after the addition of theNamedIOSystem class in PR#721, these two are largely redundant. We might also think about removing theLinearICSystem class as discussed in issue#786. But see the comments below regarding theparam keyword for a counterargument.

Also, as a note, I don't think the included unit tests actually expose the functionality that was broken (they use operator overloading, which works in the current version [I checked]).

Adding params to dynamics, output

Thedynamics() andoutput() functions forInputOutput systems were not accepting aparams keyword, which was a clear bug. I'm surprised this bug hadn't popped up before, but I think it was because we haven't been usingdynamics() andoutput() for input/output systems in any of our unit tests.

Note that_rhs and_out shouldnot takeparams as an input. Rather, these are passed by settingcurrent_params, which are then accessed by_rhs and_out. This was done this way because the ODE integration routines from SciPy don't have parameter values and so you have to store them someplace before passing_rhs toscipy.integrate.solve_ivp.

Also note that in the current implementation, aLinearIOSystem does not have parameter values since it was originally intended to just be a way to turn aStateSpace system into anInputOutputSystem while retaining the linear structure (eg, A, B, C, and D matrices). This might actually speak to a case where we want to leaveLinearIOSystem andStateSpace as separate classes, with the idea that aLinearIOSystem might have parameter values while aStateSpace system wouldn't. That would require some rewriting of the classes, since currently aLinearIOSystem is a subclass ofStateSpace, but we would need it the other way around if aStateSpace system was treated as a special case of aLinearIOSystem. I'd have to noodle on that for a bit...

PR#788 fixes up the parameter processing and adds some unit tests that expose the current bug.

Static system determination

I can't quite tell what the intention was in changing around some of the code associated with "static" systems, but in looking through the code before and after the change, I think there are some inconsistencies.

I think the definition of a "static" system should be that it is a system with no internal state. This is allowed in both theStateSpace andInputOutputSystem classes by settingnstates to zero. So, for example, a static gain can be declared via

sys1 = ct.StateSpace([]. [], [], 2)

This is different than the following declaration, which shows up in the proposed unit tests:

sys2 = ct.Statespace(0, 0, 0, 2)

Strictly speaking, sys2 isnot a static system. It is a 1 state system that is both unobservable and uncontrollable. Confusingly, the existingsys2._isstatic() method will returnTrue and the proposed unit tests tries to confirm that definition, even though I don't think it is correct.

Also, there is a another problem with the current code, which defines a system to be static if the A and B matrices are zero. That sort of makes sense in continuous time (the state would remain unchanged), but it is not actually correct in discrete time (you would want A to be the identity matrix). And even for a continuous system you could get odd behavior if the C matrix is nonzero and there is a nonzero initial condition (the system is "static", but the input/output response is now affine). I think the proper fix here is to usenstates == 0 as the proper definition of a static system.

@sawyerbfuller: Can you provide more info on what the issue was here and your thoughts on how we should define a "static" system? Depending on your response, we can figure out how to fix things.

StateSpace updates

I think the issue being addressed here is that if you try to useinterconnect with a list of systems that includes a mixture ofLinearIOSystem objects andStateSpace objects, it doesn't work correctly, even though the operator overloading for those combinations handles things properly.

The proposed fix is to update theStateSpace class to have some of the methods that normally go with anInputOutputSystem object and then use those in theLinearIOSystem object. That will work, but it messes around with the class structure in a somewhat spaghetti like way and leads to some inconsistencies. For example, the fix does not allow you tointerconnect aTransferFunction with aLinearIOSystem, even though operator overloading on that case works. In principle we could add_rhs and_out to theTransferFunction class to fix that, but of course it really doesn't make sense for a transfer function to have a right-hand side. It's just that it needs to be converted to aLinearIOSystem object via some realization (which is what the operator overloading implementation does).

I think that a better short term fix is to update theinterconnect function so that it recognizesStateSpace andTransferFunction objects and properly converts them toLinearIOSystem objects. I did that in PR#788 by adding 4 lines of code (and some more extensive unit tests).

Sounds good.

In the long term, we might want to combineStateSpace andLinearIOSystem into a single class, since after the addition of theNamedIOSystem class in PR#721, these two are largely redundant. We might also think about removing theLinearICSystem class as discussed in issue#786. But see the comments below regarding theparam keyword for a counterargument.

I agree on the idea of merging them.

Adding params to dynamics, output

Thedynamics() andoutput() functions forInputOutput systems were not accepting aparams keyword, which was a clear bug. I'm surprised this bug hadn't popped up before, but I think it was because we haven't been usingdynamics() andoutput() for input/output systems in any of our unit tests.

Yes, I agree that was a bug - by me.

Note that_rhs and_out shouldnot takeparams as an input. Rather, these are passed by settingcurrent_params, which are then accessed by_rhs and_out. This was done this way because the ODE integration routines from SciPy don't have parameter values and so you have to store them someplace before passing_rhs toscipy.integrate.solve_ivp.

I see, makes sense.

What is the difference betweenself._current_params andself.params, i.e. what is the reason thatself._update_params only updates_current_params and notparams?

Also note that in the current implementation, aLinearIOSystem does not have parameter values since it was originally intended to just be a way to turn aStateSpace system into anInputOutputSystem while retaining the linear structure (eg, A, B, C, and D matrices). This might actually speak to a case where we want to leaveLinearIOSystem andStateSpace as separate classes, with the idea that aLinearIOSystem might have parameter values while aStateSpace system wouldn't. That would require some rewriting of the classes, since currently aLinearIOSystem is a subclass ofStateSpace, but we would need it the other way around if aStateSpace system was treated as a special case of aLinearIOSystem. I'd have to noodle on that for a bit...

My inclination would be to keep things simple and assume that if somebody wants to update matrix entries, they can do that by creating new A, B, C, D matrices and creating a newStateSpace system, but there may be use cases I am not aware of.

PR#788 fixes up the parameter processing and adds some unit tests that expose the current bug.

Great : )

Static system determination

I can't quite tell what the intention was in changing around some of the code associated with "static" systems, but in looking through the code before and after the change, I think there are some inconsistencies.

I think the definition of a "static" system should be that it is a system with no internal state. This is allowed in both theStateSpace andInputOutputSystem classes by settingnstates to zero. So, for example, a static gain can be declared via

sys1 = ct.StateSpace([]. [], [], 2)

This is different than the following declaration, which shows up in the proposed unit tests:

sys2 = ct.Statespace(0, 0, 0, 2)

Strictly speaking, sys2 isnot a static system. It is a 1 state system that is both unobservable and uncontrollable. Confusingly, the existingsys2._isstatic() method will returnTrue and the proposed unit tests tries to confirm that definition, even though I don't think it is correct.

Glad this exposed a bug in how a static system is defined. Makes sense to me.

Also, there is a another problem with the current code, which defines a system to be static if the A and B matrices are zero. That sort of makes sense in continuous time (the state would remain unchanged), but it is not actually correct in discrete time (you would want A to be the identity matrix). And even for a continuous system you could get odd behavior if the C matrix is nonzero and there is a nonzero initial condition (the system is "static", but the input/output response is now affine). I think the proper fix here is to usenstates == 0 as the proper definition of a static system.

Good catch regarding discrete-time static state-space systems. I agree.

@sawyerbfuller: Can you provide more info on what the issue was here and your thoughts on how we should define a "static" system? Depending on your response, we can figure out how to fix things.

The primary motivation of which I am aware for detecting static systems is to make it convenient to connect them to either discrete-time or continuous-time systems, because they are simply a gain. Currently, anyStateSpace system that is detected to be static is set to havedt=None by default, to give this behavior.

But I imagine it is possible that there are pitfalls to this, and it is not that much to ask that a user always make suredt values are compatible. But on the other hand, without any specific examples of problems that arise out of this choice, my inclination is to make things as easy as possible and leave this behavior in.

What is the difference between self._current_params and self.params, i.e. what is the reason that self._update_params only updates _current_params and not params?

For posterity:self.params is the default parameter values when the system is defined. But you can pass new parameters when you run a simulation usinginput_output_response andself.current_params stores those values, if specified.