You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
I'll try to take a look this weekend. I see you have put the brakes on this
over at #194 [edit: oops, meant #206].
I see Matlab describe their MIMO tf pole algorithm here:
https://www.mathworks.com/help/control/ref/pole.html (see "algorithms"
near the end)
I can't remember if that's what is intended here.
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].
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.
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?
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.
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.
Activity
murrayrm commentedon Dec 30, 2017
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.repagh commentedon May 30, 2018
Does this still happen with my new implementation of _common_den ?
roryyorke commentedon May 31, 2018
roryyorke commentedon Jun 4, 2018
Here's a moderately tricky case from Maciejowski's Multivariable Control, Example 2.2:
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 commentedon Jun 4, 2018
This is system can be put into
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-1poles 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
tzeroworks exclusively with state models, the answer would always be 2 or[2, -2]depending on which nonminimal representation is given.roryyorke commentedon Jun 5, 2018
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 commentedon 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.
TransferFunction _common_den rewrite - issue #194 (#206)
murrayrm commentedon 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:
This appears to be right, but leaving for @roryyorke to confirm and then we can close out this issue.
roryyorke commentedon Jul 18, 2018
roryyorke commentedon Jul 22, 2018
Tested a007fcc, looks good to me.