TransferFunction _common_den rewrite - issue #194 #206
Conversation
|
Average coverage actually decreased, simply because I made xferfcn.py 58 lines shorter and that file had a coverage wel above average. |
|
Please do not merge yet. I now realise that only poles common to an input can be removed from the tf's, (thanks to issue #111). I will try to resolve that over the next week or so. |
control/xferfcn.py
Outdated
| # pre-calculate the poles for all num, den | ||
| # has zeros, poles, gain, list for pole indices not in den, | ||
| # number of poles known at the time analyzed | ||
| self2 = self.minreal() |
There was a problem hiding this comment.
I'm not sure about this; I think tf([1,1],[1,2,1]).pole() should return [-1, -1].
control/xferfcn.py
Outdated
| # are the same size as the denominator. | ||
| currentpoles = poleset[i][j][1] | ||
| nothave = ones(currentpoles.shape, dtype=bool) | ||
| for ip, p in enumerate(poles): |
There was a problem hiding this comment.
is this supposed to be enumerate(poleset)? I can't see any modification to poles after the initial assignment to [].
edit: ah, never mind, I see the poles.append() a bit later.
|
I think I got it this time.
Note that this relies on my fixes to Slycot; these fix a number of edge cases |
- 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
|
I fixed up the Travis CI issues in PR #210 and tested this code (locally) to make sure that it passes unit tests. On MacOS 10.13 I get the following error: I haven't figured out what specifically in this PR is causing the problem, but need to chase this down before we can merge. @repagh Can you run this against the latest version of |
|
I get the same error. However, I do not think this should be an issue. The modred is performed on a state-space system created with tf2ss, through slycot. One of the states is removed in modred, and since the tf2ss has been changed, a different set (of equally valid) states has come out of that step. Without control over what states are chosen for a state-space system, one should not try to eliminate a specific single state. Using siso_ss1 instead of self.siso_ss3 for the test works. |
of which the selection of states was automatic
|
I also see a bunch of other tests failing (e.g. test_robust). I don't get that same behaviour locally. Closer inspection of the log shows that the failing slycot builds are missing openblas; I think we need to update .travis.yml here too. |
|
Confirmed that latest change (to |
On my machines I got a consistent issue #194 error. (Mac OSX and Linux).
After playing around with the seed in convert_test.py, I got slightly different results for the tests. I inspected _common_den (xferfcn.py) with the debugger, and found that in some cases a lot of cancelling poles would be added; the sorting for pole comparison did not consistently work.
I then re-thought and re-worked the _common_den function, instead of sorting, cycling through all known poles, comparing to poles in the current denominator, marking which ones are found -- to prevent accepting double poles, and remembering which ones are not found.
The whole thing is a lot shorter, and I believe also a lot more robust. I hope this eliminates the elusive #194