BugFix: tf2ss now handles static gains #129

roryyorke · 2017-01-04T19:26:53Z

Fix for #81.

coveralls · 2017-01-04T19:29:56Z

Changes Unknown when pulling a44305b on roryyorke:rory/tf2ss-static-gain into ** on python-control:master**.

slivingston · 2017-01-06T17:52:18Z

Could you split testTf2ssStatic from a44305b#diff-34ce1aa9250b93553151bda70aa98a07 into two separate tests? Use slycot_check as the splitting boundary.

Following my comment on PR #122, tests should not partially change when Slycot is present or not. Otherwise, debugging efficiency is negatively impacted.

roryyorke · 2017-01-06T18:27:15Z

You mean using @unittest.skipIf(not slycot_check(), "slycot not installed") as elsewhere in the test suite?

Sure, will do.

roryyorke · 2017-01-08T18:53:50Z

It looks like this is actually a bug in Slycot, as speculated in [1]; I can get SLICOT's TD04AD to convert a static TF. I'll see if I can wrap my mind around f2py's syntax and fix this in Slycot. In that case we can keep the unit tests in this PR, but the change to statesp.py will be different, and the code will be much simpler.

[1] https://github.com/roryyorke/python-control/blob/5c839c205dce8e072909509545266a466d4a2404/control/statesp.py#L669-L671

slivingston · 2017-02-02T20:30:14Z

Because changes from python-control/Slycot#5 are now in Slycot, the only remaining case before we can close #81 is when Slycot is not installed.

@roryyorke Some of the changes from your original commit in this PR might be able to address the case of no slycot. In particular,

D = empty((sys.outputs,sys.inputs),dtype=float)
for i,j in itertools.product(range(sys.outputs),range(sys.inputs)):
    D[i,j] = sys.num[i][j][0] / sys.den[i][j][0]
return StateSpace([], [], [], D, sys.dt)

roryyorke · 2017-02-05T17:53:15Z

Is your idea to have three cases: (1) static gain (SISO or MIMO), (2) with-Slycot (any dynamic), (3) no-Slycot (SISO dynamic only) ?

We could add a case (0), namely empty systems (or this could be handled in (1)); this would let us sidestep the Slycot issues with empty systems you identified in in the related Slycot PR.

slivingston · 2017-02-09T04:59:05Z

Unless I am missing something, with Slycot, static gain does not need to be treated as a special case, thanks to your patches to Slycot.

However, when Slycot is not installed, static gain would be treated separately.

slivingston · 2017-02-09T05:05:51Z

so, the cases are:

with Slycot
static gains, no Slycot
SISO, no Slycot

Before creating a special case to handle empty systems (as noticed during review of python-control/Slycot#5), we might wait until there is a motivating application.

roryyorke · 2017-02-11T12:45:33Z

CI won't pass without updated Slycot; it passes on my Ubuntu 14.04 x64 system, with Conda Python 3.5.2, both with Slycot 9a098d12, and without Slycot.

slivingston · 2017-02-12T08:57:49Z

I also demonstrated success on your tests testTf2ssStaticSiso and testTf2ssStaticMimo on a GNU/Linux system using Python versions 3.5.2 and 2.7.9, each with and without the current tip of master branch Slycot, 001b45d437ae7f9e9f3ff0d82db339897d548cab

Tomorrow I will work on releasing the next version of Slycot.

coveralls · 2017-02-14T05:39:27Z

Coverage decreased (-0.1%) to 77.352% when pulling 02080e0 on roryyorke:rory/tf2ss-static-gain into 8caf661 on python-control:master.

slivingston · 2017-02-14T15:03:24Z

control/tests/convert_test.py

+        """Regression: tf2ss for MIMO static gain"""
+        import control
+        # 2x3 TFM
+        gmimo = control.tf2ss(control.tf([[ [23],   [3],  [5] ], [ [-1],  [0.125],  [101.3] ]],


How did you select these coefficients?

It's an arbitrary selection of numbers over a (small) range of magnitudes and signs; in all cases the ratios are exactly representable with low precision (see d just below), so I could use np.assert_array_equal().

slivingston · 2017-02-14T15:07:04Z

control/statesp.py

-            lti_sys = lti(squeeze(sys.num), squeeze(sys.den))
-            return StateSpace(lti_sys.A, lti_sys.B, lti_sys.C, lti_sys.D,
-                              sys.dt)
+            # No Slycot.  Scipy tf->ss can't handle MIMO, but static MIMO is an easy special case we can check for here


Comment lines should have length no more than 79 char. PEP 8 recommends 72 for the case of comments, but in my experience a uniform max line length is easier to follow.

OK, will fix. Sorry, it's pretty obvious now that I look at it. Emacs fills this to 68 columns, which in this case doesn't waste an extra line:

# No Slycot. Scipy tf->ss can't handle MIMO, but static # MIMO is an easy special case we can check for here

slivingston · 2017-02-14T16:08:19Z

control/statesp.py

+                if (sys.inputs != 1 or sys.outputs != 1):
+                    raise TypeError("No support for MIMO without slycot")
+
+                # TODO: do we want to squeeze first and check dimenations?


Should "dimenations" be "dimensions"?

I can't take the credit for this one, the comment was there already ;). Not sure it's applicable anymore: the check that sys.inputs and sys.outputs are both 1 implies that squeeze will bring num and den to 1-D?

Yes, I agree. The comment is obsolete after the addition in commit c90a399 of the check that sys.inputs = sys.outputs = 1.

I am planning to review and perform maintenance on documentation and code comments, so I can delete this comment at that time.

slivingston · 2017-02-14T16:21:15Z

As you might have noticed, a current Slycot release was made, and now the CI jobs for this PR are passing.

Aside from my request about line length and question about the static gains MIMO test case, changes here are good to go.

Check for static-gain (i.e., constant) transfer function matrix, and handle specially. Added unit tests for static SISO and MIMO plants.

coveralls · 2017-02-14T20:16:55Z

Coverage decreased (-0.1%) to 77.352% when pulling c5a8ad3 on roryyorke:rory/tf2ss-static-gain into 8caf661 on python-control:master.

slivingston self-assigned this Jan 6, 2017

roryyorke force-pushed the rory/tf2ss-static-gain branch from a44305b to 5c839c2 Compare January 6, 2017 18:58

roryyorke mentioned this pull request Jan 11, 2017

Fix: a and c matrices must have at least one column in td04ad_r python-control/Slycot#5

Merged

roryyorke force-pushed the rory/tf2ss-static-gain branch from 5c839c2 to 02080e0 Compare February 11, 2017 12:24

slivingston reviewed Feb 14, 2017

View reviewed changes

BugFix: tf2ss now handles static MIMO gains with no Slycot

c5a8ad3

Check for static-gain (i.e., constant) transfer function matrix, and handle specially. Added unit tests for static SISO and MIMO plants.

roryyorke force-pushed the rory/tf2ss-static-gain branch from 02080e0 to c5a8ad3 Compare February 14, 2017 20:13

slivingston merged commit c5a8ad3 into python-control:master Feb 15, 2017

slivingston mentioned this pull request Feb 15, 2017

tf2ss() does not seem to work for static gains #81

Closed

roryyorke deleted the rory/tf2ss-static-gain branch February 15, 2017 17:04

murrayrm added this to the 0.8.0 milestone Dec 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BugFix: tf2ss now handles static gains #129

BugFix: tf2ss now handles static gains #129

roryyorke commented Jan 4, 2017

coveralls commented Jan 4, 2017 •

edited

Loading

slivingston commented Jan 6, 2017

roryyorke commented Jan 6, 2017

roryyorke commented Jan 8, 2017

slivingston commented Feb 2, 2017

roryyorke commented Feb 5, 2017

slivingston commented Feb 9, 2017

slivingston commented Feb 9, 2017

roryyorke commented Feb 11, 2017

slivingston commented Feb 12, 2017

coveralls commented Feb 14, 2017 •

edited

Loading

slivingston Feb 14, 2017

roryyorke Feb 14, 2017

slivingston Feb 14, 2017

roryyorke Feb 14, 2017

slivingston Feb 14, 2017 •

edited

Loading

roryyorke Feb 14, 2017

slivingston Feb 15, 2017

slivingston commented Feb 14, 2017

coveralls commented Feb 14, 2017 •

edited

Loading

BugFix: tf2ss now handles static gains #129

BugFix: tf2ss now handles static gains #129

Conversation

roryyorke commented Jan 4, 2017

coveralls commented Jan 4, 2017 • edited Loading

slivingston commented Jan 6, 2017

roryyorke commented Jan 6, 2017

roryyorke commented Jan 8, 2017

slivingston commented Feb 2, 2017

roryyorke commented Feb 5, 2017

slivingston commented Feb 9, 2017

slivingston commented Feb 9, 2017

roryyorke commented Feb 11, 2017

slivingston commented Feb 12, 2017

coveralls commented Feb 14, 2017 • edited Loading

slivingston Feb 14, 2017

Choose a reason for hiding this comment

roryyorke Feb 14, 2017

Choose a reason for hiding this comment

slivingston Feb 14, 2017

Choose a reason for hiding this comment

roryyorke Feb 14, 2017

Choose a reason for hiding this comment

slivingston Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

roryyorke Feb 14, 2017

Choose a reason for hiding this comment

slivingston Feb 15, 2017

Choose a reason for hiding this comment

slivingston commented Feb 14, 2017

coveralls commented Feb 14, 2017 • edited Loading

coveralls commented Jan 4, 2017 •

edited

Loading

coveralls commented Feb 14, 2017 •

edited

Loading

slivingston Feb 14, 2017 •

edited

Loading

coveralls commented Feb 14, 2017 •

edited

Loading