TransferFunction _common_den rewrite - issue #194 by repagh · Pull Request #206 · python-control/python-control

repagh · 2018-05-29T22:59:45Z

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

coveralls · 2018-05-29T23:28:39Z

Coverage decreased (-12.7%) to 65.133% when pulling 732c924 on repagh:tf-combination into 601b581 on python-control:master.

…trol into tf-combination

repagh · 2018-05-30T05:30:31Z

Average coverage actually decreased, simply because I made xferfcn.py 58 lines shorter and that file had a coverage wel above average.

repagh · 2018-05-31T08:26:51Z

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.

murrayrm · 2018-05-31T09:20:04Z

@repagh It would be great to add a unit test that catches issue #111 while you are at it.

roryyorke · 2018-06-02T11:24:35Z

control/xferfcn.py

+        # 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()


I'm not sure about this; I think tf([1,1],[1,2,1]).pole() should return [-1, -1].

roryyorke · 2018-06-02T11:25:25Z

control/xferfcn.py

-        # are the same size as the denominator.
+                currentpoles = poleset[i][j][1]
+                nothave = ones(currentpoles.shape, dtype=bool)
+                for ip, p in enumerate(poles):


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.

repagh · 2018-06-20T21:18:56Z

I think I got it this time.

The convert_test.py smoothly runs
test case for Too few poles for MIMO tf #111 also works
I modified/corrected a test case in xferfcn_test.py, same issue, deleted poles

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

murrayrm

I've looked through these changes and they look good to merge.

murrayrm · 2018-07-01T02:03:20Z

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:

ERROR: testModred (control.tests.matlab_test.TestMatlab)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/murray/Dropbox/macosx/src/python-control/murrayrm/control/tests/matlab_test.py", line 400, in testModred
    modred(self.siso_ss3, [1], 'matchdc')
  File "/Users/murray/Dropbox/macosx/src/python-control/murrayrm/control/modelsimp.py", line 181, in modred
    raise ValueError("Matrix A22 is singular to working precision.")
ValueError: Matrix A22 is singular to working precision.

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 slycot and see if you get the same error?

repagh · 2018-07-01T21:52:40Z

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

repagh · 2018-07-01T22:36:52Z

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.

murrayrm · 2018-07-02T00:05:27Z

The other tests are failing due to changes that are addressed in PR #210. I'll test locally and if everything is working then I'll merge PR #210 and then this PR, which should get us back to a working master branch.

murrayrm · 2018-07-02T00:34:33Z

Confirmed that latest change (to modred unit tests) fixes the issue on my local machine. PR #210 is now merged, so this PR is ready to go.

repagh and others added 3 commits May 29, 2018 20:51

partial changes, not complete, to num/den combination

237597e

have replaced the _common_den function internals. Passes tests

5acb42c

Merge branch 'master' into tf-combination

05a6934

repagh added 2 commits May 30, 2018 01:37

make xferfcn.py python2 compatible again

a0810db

Merge branch 'tf-combination' of https://github.com/repagh/python-con…

f4c230c

…trol into tf-combination

repagh requested a review from murrayrm May 30, 2018 11:34

roryyorke mentioned this pull request Jun 2, 2018

Too few poles for MIMO tf #111

Closed

roryyorke reviewed Jun 2, 2018

View reviewed changes

repagh and others added 5 commits June 4, 2018 18:40

somewhere halfway

492635c

still not figured out

324c573

work in progress

0722cf3

more work in progress

dfb1507

working tf -> ss transformation now, also solved #111

50c9bf5

disabled MIMO test when no slycot

92a3354

repagh mentioned this pull request Jun 24, 2018

Release v0.8.0 #191

Closed

- do not cancel pole/zero pairs before calculating pole() in xferfcn.py

7e594c4

- 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

murrayrm added this to the 0.8.0 milestone Jun 30, 2018

murrayrm approved these changes Jul 1, 2018

View reviewed changes

- change testModred; that one did state removal on a system

732c924

of which the selection of states was automatic

murrayrm merged commit 7bed77f into python-control:master Jul 2, 2018

murrayrm mentioned this pull request Jul 2, 2018

Travis CI sporadically fails in control.tests.convert_test.TestConvert #194

Closed

bnavigator mentioned this pull request Nov 9, 2019

xferfcn_test test_pole_mimo fails on arm and powerpc #343

Closed

Comments

Conversation

repagh commented May 29, 2018

Uh oh!

coveralls commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repagh commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repagh commented May 31, 2018

Uh oh!

murrayrm commented May 31, 2018

Uh oh!

roryyorke Jun 2, 2018

Choose a reason for hiding this comment

Uh oh!

roryyorke Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repagh commented Jun 20, 2018

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

murrayrm commented Jul 1, 2018

Uh oh!

repagh commented Jul 1, 2018

Uh oh!

repagh commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murrayrm commented Jul 2, 2018

Uh oh!

murrayrm commented Jul 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented May 29, 2018 •

edited

Loading

repagh commented May 30, 2018 •

edited

Loading

roryyorke Jun 2, 2018 •

edited

Loading

repagh commented Jul 1, 2018 •

edited

Loading