Use nstates instead of nx in lsimplot

[albheim]

delay_example did not run for me, seemed to be this

[JuliaControlBot]

Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/runs/843412760?check_suite_focus=true for more details.

[olof3]

I think we are moving in the other direction actually, and that we instead should define the getproperty for .nx. And preferably deprecate nstates, noutputs, ninputs. But I'm not sure that this is the plan or if there is a plan. @mfalt @baggepinnen

I think that I prefer sys.nx since it is shorter.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (dev@0d9ec6f). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 7830922 differs from pull request most recent head 1c3cbb7. Consider uploading reports for the commit 1c3cbb7 to get more accurate results

@@          Coverage Diff           @@
##             dev     #508   +/-   ##
======================================
  Coverage       ?   84.56%           
======================================
  Files          ?       31           
  Lines          ?     3182           
  Branches       ?        0           
======================================
  Hits           ?     2691           
  Misses         ?      491           
  Partials       ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d9ec6f...1c3cbb7. Read the comment docs.

[baggepinnen]

I generally prefer nx, but a function is much easier to broadcast and I don't think we should not depreciate those since it's a hassle to broadcast getproperty. Defining properties for transfer functions would be good though.

[albheim]

Ah, okay. I thought it was going towards nstates.

After a quick look it seems that tf, sisorational and sisozpk does not have either, delaylti has nstates and ss has nx.

Could we just have all of them implement getproperty on nu, ny, nx and then have one spot where we define ninputs(sys::LtiSystem) = sys.nu and same for the other?

[mfalt]

I do prefer nstates(sys) for it's clarity in the package. But I think defining getproperty so the user can use .nx is fine.

[olof3]

a function is much easier to broadcast

Okay, then we should definitely keep them around.

After a quick look it seems that tf, sisorational and sisozpk does not have either,

States are for statespace realizations. But I guess order or something like that could be useful for transfer functions, although it feels like we are coping quite well by just looking at the length of the denominator polynomial directly. Not sure what we do for zpk

delaylti has nstates and ss has nx.

Then we should define nstates for ss if that is convenient for broadcasting.

When you bring it up, delay systems have infinitely many states, so I don't think we should define nstates or nx for those (although it was me who implemented nstates for it). Rather one should have to call that function on the statespace realization used for representing the delay system, and nstates on the DelayLTISystem should throw an error.

I do prefer nstates(sys) for it's clarity in the package.

I partly see your point, and although I in general prefer long names, I don't see any problems here. I also think it is important with clarity in the package, and for this reason I think it is important to keep things short when there is no risk of confusion and then use longer names for less obvious things.

This piece of code is a good example:

ControlSystems.jl/src/connections.jl

Line 371 in 484b00f

feedback(G, Δ, U1=G.nu-Δ.ny+1:G.nu, Y1=G.ny-Δ.nu+1:G.ny, W1=1:G.ny-Δ.nu, Z1=1:G.nu-Δ.ny, pos_feedback=true)

of course, one could have defined variables G_ny Δ_nu, but that seems quite cumbersome and could potentially introduce errors, rather than just using G.ny, Δ.nu directly.

Here is an other less extreme example, but already here i find it a lot harder to quickly see what goes on in the first version of the code.

    A = [s1.A                             zeros(T, nstates(s1), nstates(s2));
            zeros(T, nstates(s2), nstates(s1))           s2.A]

    A = [s1.A                         zeros(T, s1.nx, s2.nx);
           zeros(T, s2.nx, s1.nx)            s2.A]

The number of states also very much feels like an inherent property of a statespace realization, so to me it feels much more natural to access it as a property rather than having to call a function on the object.

[baggepinnen]

When you bring it up, delay systems have infinitely many states, so I don't think we should define nstates or nx for those (although it was me who implemented nstates for it). Rather one should have to call that function on the statespace realization used for representing the delay system, and nstates on the DelayLTISystem should throw an error.

I don't quite understand the suggestion, which statespace realization? Wouldn't it just be an annoyance and break a lot of generic code to not define nx for delay systems?

The number of states also very much feels like an inherent property of a statespace realization, so to me it feel much more natural to access it as a property rather than having to call a function on the object.

So does length, size, ndims of an array? Those are still functions in julia because functions are much more powerful than properties. The only upside with properties is visual.

[olof3]

I don't quite understand the suggestion, which statespace realization?

The class of delay systems the DelayLTISytem models is those that can be expressed as a linear fractional transformation between a statespace system P and a number of "delay buffers". The delay system has infinitely many states, while P has a finite number of stats. (Currently P is implemented with a special type PartitionedStateSpace, but as we have discussed previously , we might move away from that. But that is an unrelated issue.)

Wouldn't it just be an annoyance and break a lot of generic code to not define nx for delay systems?

If it does, then I guess that something is wrong. Now I'm curious if something breaks, so I think I'll try it out and possibly make a PR.

The number of states also very much feels like an inherent property of a statespace realization, so to me it feels much more natural to access it as a property rather than having to call a function on the object.
So does length, size, ndims of an array? Those are still functions in julia because functions are much more powerful than properties. The only upside with properties is visual.

Ah, yes, that's a fair point. We should definitely keep the function versions around.

There are a few places in the code where nstates(sys) looks slightly better, but in many cases it would look significantly worse. If we want to go for consistency then I think we should go with .nx, but otherwise we can just take what feels more natural or whatever we feel like.

In the currently discussed case I'd say .nx only looks insignificantly better, so in this case I don't mind nstates.

[baggepinnen]

The class of delay systems the DelayLTISytem models is those that can be expressed as a linear fractional transformation between a statespace system P and a number of "delay buffers". The delay system has infinitely many states, while P has a finite number of stats.

Ah, this makes sense. I was somehow only thinking about the PartitionedStateSpace type.

About the system dimensions, I find it the cleanest to introduce local variables with the sizes, e.g,

nx,nu,ny = sys.nx, sys.nu, sys.ny

and then use those in the function body. When doing that,

nx,ny,nu = nstates(sys), noutputs(sys), ninputs(sys)

would actually be more descriptive, but I often write the former since it's shorter.

[olof3]

ny, nu = size(sys)
nx = nstates(sys)

slightly shorter, although two lines.

So if one is repeatedly using the dimensions I think this is quite nice, and here I'm in slightly in favor of nstates.

However, if one introduces nu, nx etc, in a lot of places in the code, then it does not seem very unclear to just use sys.nx if one only needs it in a few places.

However, it will be quite annoying if there is more than one system.