Rory ss static pole #110

roryyorke · 2016-08-31T20:01:24Z

This is a mod on top of #108, rebased on cdd3e73. If #107 and #108 and this mod are all OK, I guess one could just take this PR and delete the others (which I haven't rebased yet).

The mod lets StateSpace.pole work with static gains; it returns np.array([]), which is both reasonable, and the behaviour as TransferFunction.pole for static gains. Without this mod, the eigval call in StateSpace.pole results in an exception when A is an empty matrix.

Regression test added.

I'm pretty sure the Python 3.3 failure on Travis CI [1] hasn't got anything to do with this.

Prompted by modelsimp.minreal bugging out when minreal()'ing a statespace object was reduced to a static gain.

[1] https://travis-ci.org/roryyorke/python-control/jobs/156620757

coveralls · 2016-08-31T20:04:46Z

Coverage decreased (-0.08%) to 75.927% when pulling a68ad6c on roryyorke:rory-ss-static-pole into cdd3e73 on python-control:master.

roryyorke · 2016-12-25T19:24:42Z

Reran this on Travis today, everything passes [1].

[1] https://travis-ci.org/roryyorke/python-control/builds/156620754

slivingston · 2016-12-25T21:22:28Z

I will review it within 12 hours.

slivingston · 2016-12-27T22:23:53Z

In 315a1ea you add the comment "TODO: use super here?" above

LTI.__init__(self, inputs=D.shape[1], outputs=D.shape[0], dt=dt)

in the definition of __init__ of StateSpace in control/statesp.py. I think that using super here will ease code maintenance because it provides a reference to the parent without naming the parent, which is one of the motivations provided in the Python documentation at https://docs.python.org/3/library/functions.html#super

slivingston · 2016-12-27T22:25:36Z

However, changing to use super is orthogonal to this pull request, so we can wait until later to do so.

slivingston · 2016-12-27T22:37:43Z

control/tests/statesp_test.py

 from control import matlab
-from control.statesp import StateSpace, _convertToStateSpace
+from control.statesp import StateSpace, _convertToStateSpace,tf2ss


Though tf2ss is added here, it is not used in this module (statesp_test.py). Am I missing something?

slivingston · 2016-12-27T22:41:33Z

@roryyorke Did you write "BugFix?" in the first commit to indicate that the change might be thought of as adding a new feature, instead of fixing a bug?

slivingston · 2016-12-27T23:12:29Z

For posterity and because I closed #107 in favor of this PR, I want to reference the comment #107 (comment) from there, which raises concerns about numpy.matrix([]) being a 1\times 0 matrix instead of 0\times 0. A solution proposed in that comment and in one of the commits of that PR (e640b17) is to create a wrapper around numpy.matrix.

slivingston · 2016-12-28T00:04:16Z

Regarding the behavior of numpy.matrix([]), I did not find any written justification. It might be worthwhile for someone else to also search.

I studied the constructor code for matrix objects, which shows that numpy.matrix([]) returns a value with shape (1,0) because

none of the isinstance calls match for [] (type list);
[] is passed through numpy.array, which returns an ndarray of shape (0,) [1];
np.array([]).ndim == 1, so the matrix object is given shape = (1, np.array([]).shape[0]) [2].

Thus, I guess that one design motivation is to map 1-dimensional arrays to row matrices, without specially treating empty arrays.

[1] line 270 of numpy/matrixlib/defmatrix.py, https://github.com/numpy/numpy/blob/2498b74420074dffa3a062ab7fc69dfeb88a5050/numpy/matrixlib/defmatrix.py#L270
[2] lines 277-278 of numpy/matrixlib/defmatrix.py, https://github.com/numpy/numpy/blob/2498b74420074dffa3a062ab7fc69dfeb88a5050/numpy/matrixlib/defmatrix.py#L277-L278

slivingston · 2016-12-28T00:32:21Z

My recommendations:

Assert self.states == 0 in test_remove_useless_states of statesp_test.py because StateSpace._remove_useless_states contains the assignment self.states = self.A.shape[0], and one consequence of changes from your pull request is that self.states = 0 when all states are useless (instead of self.states = 1 from before).
Remove the new import of tf2ss in statesp_test.py. If you can quickly rebase commits so as to entirely avoid the noise (i.e., accidental addition, then removal), please do. Otherwise, OK to just add a correction commit.

Once these are addressed, this PR is ready to be merged.

roryyorke · 2016-12-28T17:59:17Z

Thanks for the thorough review! I'll put all my responses here.

TODO for super: I think we agree this change doesn't belong in this PR; I suppose the Right Thing would have been to open a new issue.

"BugFix?": indeed, whether this is a bugfix or enhancement is not very clear. I can edit the log (I guess? never done that with git) if you like.

On empty SS objects: I have implemented this, unfortunately didn't push it to the PR. It follows the proposed solution. See [1].

tf2ss: I don't remember why that's there; some sort of exploratory debugging, I guess. Will remove. I'll see if I can do the rebase you suggest.

I'll add the recommended assertion, thanks.

OK, so for now I will:

rebase this on c498d3d
remove the ? from the "BugFix?" log message
remove the unnecessary tf2ss import
add the extra assertion

The empty SS object extension of [1] is on the same theme as the rest of the PR, but would require additional review. I could either add it here, or open a new PR for it.

[1] roryyorke@ba75ae5

…cts. Allows StateSpace([],[],[],D), which failed previously. Static gains have sizes enforced as follows: A 0-by-0, B 0-by-ninputs, C noutputs-by-0. Tests added for instantiation, and sum, product, feedback, and appending, of 1x1, 2x3, and 3x2 static gains StateSpace objects.

On Python 2.7, the special case "all states useless" in _remove_useless_states resulted in A=[[0]] (and similarly for B and C). The special case is no longer needed, since empty A, B, C matrices can be handled. numpy.delete does the right thing w.r.t. matrix sizes (e.g., deleting all columns of a nxm matrix gives an nx0 matrix). Added test for this.

Do this by only calling Slycot's tb01pd for non-static systems.

coveralls · 2016-12-28T18:31:07Z

Changes Unknown when pulling c84debb on roryyorke:rory-ss-static-pole into ** on python-control:master**.

roryyorke · 2016-12-28T18:31:20Z

I squashed the "messagefix" commit in the rebase, otherwise changes as in previous comment.

roryyorke · 2016-12-28T18:31:46Z

I squashed the messagefix commit, otherwise rebase is as described in previous comment.

#110 Changes are from branch `rory-ss-static-pole` of https://github.com/roryyorke/python-control.git

slivingston · 2016-12-28T19:59:02Z

Thanks; the rebase is good. To better organize discussion, I decided to leave consideration of a wrapper of numpy.matrix([]) (as in #110 (comment) and roryyorke@ba75ae5) to another issue or PR.

#101 Changes are from branch `master` of https://github.com/mp4096/python-control.git There was merge conflict in how a for-loop was refactored into `map` (here) vs. list comprehension (from PR #110). I compared the two alternatives using %timeit of Jupyter for matrices that would correspond to LTI systems with 10 state dimensions, 2 inputs, 2 outputs (so, the A matrix has shape (10, 10), B matrix has shape (10,2), etc.), and with 100 state dimensions, 20 inputs, 20 outputs, all using matrices from numpy.random.random((r,c)). The difference in timing performance does not appear significant. However, the case of `map` was slightly faster (approximately 500 to 900 ns less in duration), so I decided to use that one to resolve the merge conflict.

slivingston self-assigned this Sep 11, 2016

slivingston reviewed Dec 27, 2016

View reviewed changes

This was referenced Dec 27, 2016

Allow static gain StateSpace objects to be straightforwardly created. #107

Closed

Allow minreal to work with static gain StateSpace objects #108

Closed

roryyorke added 4 commits December 28, 2016 20:12

BugFix: allow minreal on static gain StateSpace objects

09f9bab

Do this by only calling Slycot's tb01pd for non-static systems.

BugFix: pole of stateless StateSpace object is empty array

c84debb

roryyorke force-pushed the rory-ss-static-pole branch from a68ad6c to c84debb Compare December 28, 2016 18:28

slivingston merged commit c84debb into python-control:master Dec 28, 2016

slivingston added a commit that referenced this pull request Dec 28, 2016

Merge pull request #110

b6b9906

#110 Changes are from branch `rory-ss-static-pole` of https://github.com/roryyorke/python-control.git

slivingston mentioned this pull request Dec 28, 2016

Consider using super idiom for child classes #124

Closed

roryyorke deleted the rory-ss-static-pole branch December 29, 2016 17:36

slivingston mentioned this pull request Dec 30, 2016

Refactored matrix inversions (see #85 and #91) #101

Merged

roryyorke mentioned this pull request Jan 1, 2017

_remove_useless_states() can change types to ndarrays #82

Closed

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

Rory ss static pole #110

Rory ss static pole #110

roryyorke commented Aug 31, 2016

coveralls commented Aug 31, 2016 •

edited

Loading

roryyorke commented Dec 25, 2016

slivingston commented Dec 25, 2016

slivingston commented Dec 27, 2016

slivingston commented Dec 27, 2016

slivingston Dec 27, 2016

slivingston commented Dec 27, 2016

slivingston commented Dec 27, 2016

slivingston commented Dec 28, 2016

slivingston commented Dec 28, 2016

roryyorke commented Dec 28, 2016

coveralls commented Dec 28, 2016 •

edited

Loading

roryyorke commented Dec 28, 2016

roryyorke commented Dec 28, 2016

slivingston commented Dec 28, 2016

Rory ss static pole #110

Rory ss static pole #110

Conversation

roryyorke commented Aug 31, 2016

coveralls commented Aug 31, 2016 • edited Loading

roryyorke commented Dec 25, 2016

slivingston commented Dec 25, 2016

slivingston commented Dec 27, 2016

slivingston commented Dec 27, 2016

slivingston Dec 27, 2016

Choose a reason for hiding this comment

slivingston commented Dec 27, 2016

slivingston commented Dec 27, 2016

slivingston commented Dec 28, 2016

slivingston commented Dec 28, 2016

roryyorke commented Dec 28, 2016

coveralls commented Dec 28, 2016 • edited Loading

roryyorke commented Dec 28, 2016

roryyorke commented Dec 28, 2016

slivingston commented Dec 28, 2016

coveralls commented Aug 31, 2016 •

edited

Loading

coveralls commented Dec 28, 2016 •

edited

Loading