JuliaControl / ControlSystems.jl

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

Change polynomial implementation to Polynomials.jl #22

Closed mfalt closed 3 years ago

mfalt commented 8 years ago

From @mfalt on February 21, 2016 1:7

The implementation of Poly is outdated and the roots function extremely inaccurate. We should consider using the Polynomial.jl package.

Copied from original issue: mfalt/Control.jl#9

aytekinar commented 8 years ago

We think the same with @neveritt. Since we already require the Polynomials package, maybe we can internally save everything in Polynomials.Poly type and write methods that would convert calls tf(num::Array, den::Array) from the natural way of writing transfer functions to the internal (reversed order) representation of Polynomials.Poly.

We can have a look into this if you agree. If you give us whereabouts of the functions that use Poly, that would be great. Otherwise, we will check the files accordingly.

mfalt commented 8 years ago

Yes, I agree, the interface should not change for the user, it should still be the highest power first. I am not sure where all the functions that use Poly are, but most of them will be in the types/ folder, mainly in sisotf.jl and sisozpk.jl . I think we should try to keep almost all access to the polinomials in the respective types and then provide a consistent interface to the types that can be accessed from all other methods. For example by implementing numvec/denvec numpoly/denpolythat for each type that returns vectors/polynomials instead of accessing mytype.num.

aytekinar commented 8 years ago

So you mean we should implement numvec/denvec to get the polynomial coefficients from highest to lowest degree and numpoly/denpoly to get the actual Polynomials.Poly object from the transfer functions?

In this way, we can have methods for tf accepting vectors to construct transfer functions from the highest to lowest degree polynomial coefficients, and those accepting Polynomials.Poly to construct from the lowest to highest degre coefficients?

mfalt commented 8 years ago

Yes, that is pretty much what I mean. Since we have a couple of different implementations of siso transferfunctions, like tf and zpk, we should try not to assume any kind of underlying structure when writing higher level code.

I have actually not thought about accepting Polynomials.Poly in constructors, but this could be a nice feature. However, I think that we should refrain from actually supplying any variables of Polinomials type to the user unless explicitly asked (for example through a numpoly/denpoly call)

aytekinar commented 8 years ago

In the current implementation of Polynomials.Poly, one can use Polynomials.Poly([1, 0, 1], "s") to get 1 + s^2. That's why we thought it would be nice to accept Polynomials.Poly inputs to construct transfer functions.

As for the last part of your message, we totally agree that we should separate the interface from the actual implementation like you suggested (i.e., unless explicitly asked for).

mfalt commented 8 years ago

Yes I agree, this could be useful if the user is interacting with any other packages that use Polynimials.