TransferFunction _common_den rewrite - issue #194 #206

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TransferFunction _common_den rewrite - issue #194 #206

TransferFunction _common_den rewrite - issue #194 #206

repagh commented May 29, 2018

coveralls commented May 29, 2018 •

edited

Loading

repagh commented May 30, 2018 •

edited

Loading

repagh commented May 31, 2018

murrayrm commented May 31, 2018

roryyorke Jun 2, 2018

roryyorke Jun 2, 2018 •

edited

Loading

repagh commented Jun 20, 2018

murrayrm left a comment

murrayrm commented Jul 1, 2018

repagh commented Jul 1, 2018

repagh commented Jul 1, 2018 •

edited

Loading

murrayrm commented Jul 2, 2018

murrayrm commented Jul 2, 2018

TransferFunction _common_den rewrite - issue #194 #206

TransferFunction _common_den rewrite - issue #194 #206

Conversation

repagh commented May 29, 2018

coveralls commented May 29, 2018 • edited Loading

repagh commented May 30, 2018 • edited Loading

repagh commented May 31, 2018

murrayrm commented May 31, 2018

roryyorke Jun 2, 2018

Choose a reason for hiding this comment

roryyorke Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

repagh commented Jun 20, 2018

murrayrm left a comment

Choose a reason for hiding this comment

murrayrm commented Jul 1, 2018

repagh commented Jul 1, 2018

repagh commented Jul 1, 2018 • edited Loading

murrayrm commented Jul 2, 2018

murrayrm commented Jul 2, 2018

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