JuliaControl / ControlSystems.jl

A Control Systems Toolbox for Julia
https://juliacontrol.github.io/ControlSystems.jl/stable/
Other
516 stars 86 forks source link

Dataformat returned by `bode` and `margin` seems sub-optimal #390

Closed KronosTheLate closed 4 years ago

KronosTheLate commented 4 years ago

The basic problem

The data returned by bode() and margin() seems like it is more nested and inconvenient than nessecary to me. Below is a screenshot of what the functions return in a simple MWE: image

Margin returns 2D arrays for each item returned. It prints okay, but can be a hassle to extract the value itself as exemplified later.

Bode returns the amplification and phase-shift as 3D arrays, and the frequencies as a 1D array. It prints poorly, which is fine because who would ever look at that data outside of a plot. But the multidimensionality can make it less-convenient-than-nessesary (I think?) to extract the data.

A basic solution

Could the returned arrays not all be 1D? Are there edge-cases I have not encountered where the data has to be multi-dimensional? If that is the case, would it be viable to return 1D arrays when it is all that is needed, or would changing the returned data-format in some cases be too inconsistent?

Ideal solution IMO - DataFrames

First of all, I guess this solution would only really apply if it is possible to make the arrays returned 1D? But in case it can be done, here goes:

Dependencies are not great, and this package is already a little heavy (In my experience, based on pre-compilation time and using time). But I am dreaming of having these functions return dataframes instead of arrays (There are probably a even more functions in here that could benefit from returning dataframes that I have not used yet).

This would make it viable to create a single variable to hold all the 3 or 4 arrays returned, with the possibility of nicely accessing the data with the variable.coloumname syntax. Currently, one has to index 3 times into the data returned from margin() to access a value, in the following manner:margin(TF)[1][1][1] or margin(TF)[1][1, 1]. It would be lovely to be able to do something like margin(TF).phasemargin and margin(TF).phasemarginfreq. I find letters so much more preferable to brackets and numbers.

But even more important, non-experienced users could figure out what the returned data represents without consulting the docstring multiple times, as I recently have. It can be especially cryptic when the return-value is not assigned to multiple variables, and becomes wrapped in a tuple as well. Then, there are multi-dimensional arrays inside a tuple, and nothing is labeled. It would be awesome to have human-readable column names that label everything right there.

mfalt commented 4 years ago

The reson for this behavior is that systems can generally have multiple inputs and multiple outputs. Thus, bode and similar functions return a response of dimension (length(w), ny, nu), where ny,nu are number of outputs and inputs. It would be possible to reduce it to a 1D array when the system is SISO, but that would have either of the following "problems"

  1. The types of the outputs changes depending on other things than the types of inputs (i.e. the dimension of the internal system matrix). This is a problem for speed though type-instability.
  2. We could implement a special type for SISO systems. This has been discussed, but we didn't like that solution.

Instead, we implemented a macro @autovec that generates some functions that can be used when the systems are known to be SISO. So your problem can be solved by calling bodev intead of bode. The other functions it has been implemented for is nyquistv and sigmav.

We could probably add this for margin too, this recent PR implements it for freqresp and makes some other minor changes.

Margin also has the ability to return multiple delay and gain margins however, so there might be some minor problems there. And margin already seem to suffer from the type-instability I mentioned before.

Edit: A small tip: There is probably no reason to use frac as you do, 3/((s+1)*(0.5s+1)) should work fine.

KronosTheLate commented 4 years ago

Aaaah. Right. We do not mess with MIMO systems in my class.

Appending v to deal with SISO systems is nice. That should probably be added to the documentation ^_^

Edit: A small tip: There is probably no reason to use frac as you do, 3/((s+1)*(0.5s+1)) should work fine.

Ye I see that in this case it is not nessecary. I just find it really convenient for nested expressions and to make fractions have a visual signature to distinguish it from other paranthesis and /'s

KronosTheLate commented 4 years ago
  1. We could implement a special type for SISO systems. This has been discussed, but we didn't like that solution.

Could you give me a recap of why creating a type for SISO was abandoned? Possibly a link to the relevant discussion. Because I was thinking about this issue, and incorperating SISO somewhere (high up I think?) in the type hirearchy seemed like a good solution to me. And because transfer functions are only SISO (right? I was told this in class), all transfer functions would simply be subtypes of SISO.

OR I am just realizing that if all transfer functions are SISO, why not just dispatch on TransferFunction with that knowledge? No new types, just a method added for TransferFunction.

olof3 commented 4 years ago

Could you give me a recap of why creating a type for SISO was abandoned?

Because we felt that the gains were not great enough to motivate additional an type arguments and additional code to maintain. There is the autovec alternative if someone would feel that this is a great problem. We have discussed this at great lengths, you should be able to find an issue from some 2-3 years ago or so. See also https://github.com/JuliaControl/ControlSystems.jl/wiki/Restructuring-ControlSystems

And because transfer functions are only SISO

The MIMO equivalent is perhaps most correctly referred to as transfer function matrices, but we refer to them as TransferFunction. In this instance I don't think that a longer name is motivated by the extremely tiny bit of extra clarity.

KronosTheLate commented 4 years ago

That seems to answer my question completely - thank you for the link