Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Too few poles for MIMO tf #111

Closed
roryyorke opened this issue Sep 8, 2016 · 10 comments
Closed

Too few poles for MIMO tf #111

roryyorke opened this issue Sep 8, 2016 · 10 comments
Assignees
Labels

Comments

@roryyorke
Copy link
Contributor

2x2 MIMO system I/s (diagonal, integrators on diagonal) is reported as having 1 pole. Converting to state-space gives the correct number of poles.

tested with slycot python-control/Slycot@a1f43ab and python-control cdd3e73

import numpy as np
import control

num = [ [ [1], [0] ],
        [ [0], [1] ] ]

den = [ [ [1,0], [1] ],
        [ [1],   [1,0] ] ]

g = control.tf(num,den)
print('g.pole()',g.pole())
print('control.ss(g).pole()',control.ss(g).pole())

output:

g.pole() [ 0.]
control.ss(g).pole() [-0. -0.]
@murrayrm
Copy link
Member

Took a quick look at this. The problems seems to be in control.TransferFunction._common_den. The method used to put together the common denominator for a MIMO transfer function is to create a list of all unique poles that are found. This obviously won't work in the case of repeated poles. Definite bug.

@murrayrm murrayrm added the bug label Dec 30, 2017
@repagh
Copy link
Member

repagh commented May 30, 2018

Does this still happen with my new implementation of _common_den ?

@roryyorke
Copy link
Contributor Author

roryyorke commented May 31, 2018 via email

@roryyorke
Copy link
Contributor Author

Here's a moderately tricky case from Maciejowski's Multivariable Control, Example 2.2:

num = [ [ [1],       [-1]      ],
        [ [1,1,-4],  [2,-1,-8] ],
        [ [1,-2],    [2,-4] ] ]

den = [ [ [1,3,2],   [1,3,2] ],
        [ [1,3,2],   [1,3,2] ],
        [ [1,1],     [1,1]   ]  ]

g = control.tf(num, den)

This has three poles: two at -1, and one at -2. It also has a transmission zero at +2, which might be of interest as a test case for #205.

Matlab's pole(tf(num,den)) gives poles [-2,-2,-1,-1,-1] for this system (but pole(minreal(ss(tf(num,den)))) gives the right answer).

Octave's control toolbox version 3.0.0 resorts to minreal(ss(g)) for MIMO poles; for this example, it gives [-1,-1,-2,-2]. If I do the same thing in python-control, I get [-1,-1,-2].

@ilayn
Copy link

ilayn commented Jun 4, 2018

This is system can be put into

[ 1/(s^2+3s+2)                                   ] * [1 -1]
[               (s^2+s-4)/(s^2+3s+2)             ]   [1  2]
[                                     (s-2)/(s+1)]   [1  1]

Then we have 5 poles indeed. However if we decompose to the other side by MFD then we get the four pole answer by adding (s+2)/(s+2) to the last row elements. But that adds a spurious zero at -2.

So we shift the problem from pole to zero in terms of minimality. The actual answer is only possible to find if the common denominator can be completed to [1, 4, 5, 2] by taking into account that the -1 poles have different directions but that needs a significantly more tedious search otherwise the problem would be giving more poles than the actual, instead of too few. Hence for Transfer models I think this is as good as it gets as far as I can see.

Since tzero works exclusively with state models, the answer would always be 2 or [2, -2] depending on which nonminimal representation is given.

@roryyorke
Copy link
Contributor Author

Is an algorithm based on extracting common denominators to left (row-wise) or right (column-wise) (or both?) a reasonable option? Or should we just convert to state-space and get the poles that way?

@ilayn
Copy link

ilayn commented Jun 8, 2018

Unfortunately, I am not aware of any algorithms that solves this satisfactorily for transfer models by obtaning Mcmillan forms. As your example nicely demonstrates, there is a need to evaluate the pole directions and I am not sure that it is worth the effort to bother with all pathological cases.

However, the initial example clearly has a bug of not taking into account the multiplicities as Richard mentioned. I guess having more poles than the Mcmillan degree is OK since nonminimality is a common expectation. But fewer poles is not correct in my opinion and might be misleading.

@murrayrm murrayrm added this to the 0.8.0 milestone Jun 30, 2018
murrayrm pushed a commit that referenced this issue Jul 2, 2018
* replaced the _common_den function internals. Passes tests
* make xferfcn.py python2 compatible again
* working tf -> ss transformation now, also solved #111
* disabled MIMO test when no slycot
* do not cancel pole/zero pairs before calculating pole() in xferfcn.py
- for the above reason, do conversion on minreal'd xferfcn in statesp.py
- add a test for not canceling pole/zero pairs when calculating pole()
- add import of matlab in discrete_test.py
* change testModred; that one did state removal on a system of which the selection of states was automatic
@murrayrm
Copy link
Member

murrayrm commented Jul 2, 2018

PR #206 claims to have fixed this problem. It definitely fixes the problem identified by @roryyorke at the top of this issue. For the "tricky case from Maciejowski's Multivariable Control, Example 2.2", I get the following:

In [4]: control.pole(g)
Out[4]: array([-2., -1., -2., -1.])

In [5]: control.pole(control.minreal(control.ss(g)))
0 states have been removed from the model
Out [5]: array([-2., -1., -1.])

This appears to be right, but leaving for @roryyorke to confirm and then we can close out this issue.

@murrayrm murrayrm removed this from the 0.8.0 milestone Jul 4, 2018
@roryyorke
Copy link
Contributor Author

roryyorke commented Jul 18, 2018 via email

@roryyorke
Copy link
Contributor Author

Tested a007fcc, looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants