NotificationsYou must be signed in to change notification settings
Fork441
Star1.8k

BugFix: tf2ss now handles static gains#129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

slivingston merged 1 commit intopython-control:masterfromroryyorke:rory/tf2ss-static-gain

Feb 15, 2017

Merged

BugFix: tf2ss now handles static gains#129

slivingston merged 1 commit intopython-control:masterfromroryyorke:rory/tf2ss-static-gain

Feb 15, 2017

Conversation

Copy link

Contributor

roryyorke commentedJan 4, 2017

Fix for#81.

Copy link

coveralls commentedJan 4, 2017•
edited
Loading

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

slivingston self-assigned this

Jan 6, 2017

Copy link

Member

slivingston commentedJan 6, 2017

Could you splittestTf2ssStatic froma44305b#diff-34ce1aa9250b93553151bda70aa98a07 into two separate tests? Useslycot_check as the splitting boundary.

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

Copy link

ContributorAuthor

roryyorke commentedJan 6, 2017

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

Sure, will do.

roryyorke force-pushed therory/tf2ss-static-gain branch froma44305b to5c839c2Compare

January 6, 2017 18:58

Copy link

ContributorAuthor

roryyorke commentedJan 8, 2017

It looks like thisis actually a bug in Slycot, as speculated in [1]; I can get SLICOT'sTD04AD 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 tostatesp.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

roryyorke mentioned this pull request

Jan 11, 2017

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

Merged

Copy link

Member

slivingston commentedFeb 2, 2017

Because changes frompython-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 noslycot. In particular,

D=empty((sys.outputs,sys.inputs),dtype=float)fori,jinitertools.product(range(sys.outputs),range(sys.inputs)):D[i,j]=sys.num[i][j][0]/sys.den[i][j][0]returnStateSpace([], [], [],D,sys.dt)

Copy link

ContributorAuthor

roryyorke commentedFeb 5, 2017

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 inin the related Slycot PR.

Copy link

Member

slivingston commentedFeb 9, 2017

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.

Copy link

Member

slivingston commentedFeb 9, 2017

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 ofpython-control/Slycot#5), we might wait until there is a motivating application.

roryyorke force-pushed therory/tf2ss-static-gain branch from5c839c2 to02080e0Compare

February 11, 2017 12:24

Copy link

ContributorAuthor

roryyorke commentedFeb 11, 2017

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

Copy link

Member

slivingston commentedFeb 12, 2017

I also demonstrated success on your teststestTf2ssStaticSiso andtestTf2ssStaticMimo on a GNU/Linux system using Python versions 3.5.2 and 2.7.9, each with and without the current tip ofmaster branch Slycot,001b45d437ae7f9e9f3ff0d82db339897d548cab

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

Copy link

coveralls commentedFeb 14, 2017•
edited
Loading

Coverage decreased (-0.1%) to 77.352% when pulling02080e0 on roryyorke:rory/tf2ss-static-gain into8caf661 on python-control:master.

slivingston reviewed

Feb 14, 2017

View reviewed changes

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] ]],

Copy link

Member

slivingstonFeb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

How did you select these coefficients?

Copy link

ContributorAuthor

roryyorkeFeb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 reviewed

Feb 14, 2017

View reviewed changes

control/statesp.py Outdated

		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

Copy link

Member

slivingstonFeb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

roryyorkeFeb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 reviewed

Feb 14, 2017

View reviewed changes

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?

Copy link

Member

slivingstonFeb 14, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should "dimenations" be "dimensions"?

Copy link

ContributorAuthor

roryyorkeFeb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 squeezewill bring num and den to 1-D?

Copy link

Member

slivingstonFeb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, I agree. The comment is obsolete after the addition in commitc90a399 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.

Copy link

Member

slivingston commentedFeb 14, 2017

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.

BugFix: tf2ss now handles static MIMO gains with no Slycot

c5a8ad3

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

roryyorke force-pushed therory/tf2ss-static-gain branch from02080e0 toc5a8ad3Compare

February 14, 2017 20:13

Copy link

coveralls commentedFeb 14, 2017•
edited
Loading

Coverage decreased (-0.1%) to 77.352% when pullingc5a8ad3 on roryyorke:rory/tf2ss-static-gain into8caf661 on python-control:master.

slivingston merged commitc5a8ad3 intopython-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 the0.8.0 milestone

Dec 27, 2017

Labels

None yet

4 participants

Movatterモバイル変換

BugFix: tf2ss now handles static gains#129

BugFix: tf2ss now handles static gains#129

Uh oh!

Conversation

roryyorke commentedJan 4, 2017

Uh oh!

coveralls commentedJan 4, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

slivingston commentedJan 6, 2017

Uh oh!

roryyorke commentedJan 6, 2017

Uh oh!

roryyorke commentedJan 8, 2017

Uh oh!

slivingston commentedFeb 2, 2017

Uh oh!

roryyorke commentedFeb 5, 2017

Uh oh!

slivingston commentedFeb 9, 2017

Uh oh!

slivingston commentedFeb 9, 2017

Uh oh!

roryyorke commentedFeb 11, 2017

Uh oh!

slivingston commentedFeb 12, 2017

Uh oh!

coveralls commentedFeb 14, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

slivingstonFeb 14, 2017

Choose a reason for hiding this comment

Uh oh!

roryyorkeFeb 14, 2017

Choose a reason for hiding this comment

Uh oh!

slivingstonFeb 14, 2017

Choose a reason for hiding this comment

Uh oh!

roryyorkeFeb 14, 2017

Choose a reason for hiding this comment

Uh oh!

slivingstonFeb 14, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryyorkeFeb 14, 2017

Choose a reason for hiding this comment

Uh oh!

slivingstonFeb 15, 2017

Choose a reason for hiding this comment

Uh oh!

slivingston commentedFeb 14, 2017

Uh oh!

coveralls commentedFeb 14, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coveralls commentedJan 4, 2017•
edited
Loading

coveralls commentedFeb 14, 2017•
edited
Loading

slivingstonFeb 14, 2017•
edited
Loading

coveralls commentedFeb 14, 2017•
edited
Loading