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 the connect function.

Coverage decreased (-0.002%) to 94.765% when pulling 98fba6e on sawyerbfuller:interconnect-ss into beb629b 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 the InputOutputSystem class into the StateSpace class. But that is what the LinearIOSystem class is for => we are essentially replicating what is already there.

The StateSpace class is intended to be a "low level" class that is typically not seen by the user (if you use the ss factory function, you get an LinearIOSystem object, which inherits from both InputOutputSystem and StateSpace). The StateSpace 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 the LinearICSystem class, as described in issue #786. Some alternatives would be to "eliminate" the StateSpace class completely (probably by just having it be another name for LinearIOSystem) or converting a StateSpace object to a LinearIOSystem object for any operation with an InputOutputSystem 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 the InputOutputSystem class into the StateSpace class. But that is what the LinearIOSystem class is for => we are essentially replicating what is already there.
The StateSpace class is intended to be a "low level" class that is typically not seen by the user (if you use the ss factory function, you get an LinearIOSystem object, which inherits from both InputOutputSystem and StateSpace). The StateSpace 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 that StateSpace should be a low-level class, the recent commit leaves _rhs and _out in StateSpace but takes them out of LinearIOSystem. Instead, references to the methods already in StateSpace are added in to to override the empty stubs that are defined in InputOutputSystem. This is consistent with the idea that StateSpace 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 the LinearICSystem class, as described in issue #786. Some alternatives would be to "eliminate" the StateSpace class completely (probably by just having it be another name for LinearIOSystem) or converting a StateSpace object to a LinearIOSystem object for any operation with an InputOutputSystem object.

Might make sense to eventually merge StateSpace and LinearIOSystem 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 that interconnect will allow LinearIOSystem and StateSpace objects to be combined correctly.
2. Add params as a keyword to dynamics and output.
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 use interconnect with a list of systems that includes a mixture of LinearIOSystem objects and StateSpace objects, it doesn't work correctly, even though the operator overloading for those combinations handles things properly.

The proposed fix is to update the StateSpace class to have some of the methods that normally go with an InputOutputSystem object and then use those in the LinearIOSystem 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 to interconnect a TransferFunction with a LinearIOSystem, even though operator overloading on that case works. In principle we could add _rhs and _out to the TransferFunction 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 a LinearIOSystem object via some realization (which is what the operator overloading implementation does).

I think that a better short term fix is to update the interconnect function so that it recognizes StateSpace and TransferFunction objects and properly converts them to LinearIOSystem 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 combine StateSpace and LinearIOSystem into a single class, since after the addition of the NamedIOSystem class in PR #721, these two are largely redundant. We might also think about removing the LinearICSystem class as discussed in issue #786. But see the comments below regarding the param 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

The dynamics() and output() functions for InputOutput systems were not accepting a params 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 using dynamics() and output() for input/output systems in any of our unit tests.

Note that _rhs and _out should not take params as an input. Rather, these are passed by setting current_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 to scipy.integrate.solve_ivp.

Also note that in the current implementation, a LinearIOSystem does not have parameter values since it was originally intended to just be a way to turn a StateSpace system into an InputOutputSystem while retaining the linear structure (eg, A, B, C, and D matrices). This might actually speak to a case where we want to leave LinearIOSystem and StateSpace as separate classes, with the idea that a LinearIOSystem might have parameter values while a StateSpace system wouldn't. That would require some rewriting of the classes, since currently a LinearIOSystem is a subclass of StateSpace, but we would need it the other way around if a StateSpace system was treated as a special case of a LinearIOSystem. 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 the StateSpace and InputOutputSystem classes by setting nstates 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 is not a static system. It is a 1 state system that is both unobservable and uncontrollable. Confusingly, the existing sys2._isstatic() method will return True 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 use nstates == 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 use interconnect with a list of systems that includes a mixture of LinearIOSystem objects and StateSpace objects, it doesn't work correctly, even though the operator overloading for those combinations handles things properly.

The proposed fix is to update the StateSpace class to have some of the methods that normally go with an InputOutputSystem object and then use those in the LinearIOSystem 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 to interconnect a TransferFunction with a LinearIOSystem, even though operator overloading on that case works. In principle we could add _rhs and _out to the TransferFunction 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 a LinearIOSystem object via some realization (which is what the operator overloading implementation does).

I think that a better short term fix is to update the interconnect function so that it recognizes StateSpace and TransferFunction objects and properly converts them to LinearIOSystem 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 combine StateSpace and LinearIOSystem into a single class, since after the addition of the NamedIOSystem class in PR #721, these two are largely redundant. We might also think about removing the LinearICSystem class as discussed in issue #786. But see the comments below regarding the param keyword for a counterargument.

I agree on the idea of merging them.

Adding params to dynamics, output

The dynamics() and output() functions for InputOutput systems were not accepting a params 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 using dynamics() and output() for input/output systems in any of our unit tests.

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

Note that _rhs and _out should not take params as an input. Rather, these are passed by setting current_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 to scipy.integrate.solve_ivp.

I see, makes sense.

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?

Also note that in the current implementation, a LinearIOSystem does not have parameter values since it was originally intended to just be a way to turn a StateSpace system into an InputOutputSystem while retaining the linear structure (eg, A, B, C, and D matrices). This might actually speak to a case where we want to leave LinearIOSystem and StateSpace as separate classes, with the idea that a LinearIOSystem might have parameter values while a StateSpace system wouldn't. That would require some rewriting of the classes, since currently a LinearIOSystem is a subclass of StateSpace, but we would need it the other way around if a StateSpace system was treated as a special case of a LinearIOSystem. 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 new StateSpace 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 the StateSpace and InputOutputSystem classes by setting nstates 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 is not a static system. It is a 1 state system that is both unobservable and uncontrollable. Confusingly, the existing sys2._isstatic() method will return True 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 use nstates == 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, any StateSpace system that is detected to be static is set to have dt=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 sure dt 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 using input_output_response and self.current_params stores those values, if specified.