Rory ss static pole

[roryyorke]

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]

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

[roryyorke]

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

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

[slivingston]

I will review it within 12 hours.

[slivingston]

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]

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

[slivingston]

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

[slivingston]

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

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]

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

1. none of the isinstance calls match for [] (type list);
2. [] is passed through numpy.array, which returns an ndarray of shape (0,) [1];
3. 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]

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]

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

[coveralls]

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

[roryyorke]

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

[roryyorke]

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

[slivingston]

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.