BugFix: DC gain for discrete-time systems #126
Conversation
|
Changes Unknown when pulling 3997637 on roryyorke:rory/discr-time-dcgain-fix into ** on python-control:master**. |
3997637 to
84033fe
Compare
|
Changes Unknown when pulling 84033fe on roryyorke:rory/discr-time-dcgain-fix into ** on python-control:master**. |
|
rebased on 32f13bc. Wonder why coveralls giving these "changes unknown" results? |
|
regarding the mysterious "changes unknown" from Coveralls, it might be a bug in Coveralls server side. |
| return np.squeeze(gain) | ||
|
|
||
|
|
There was a problem hiding this comment.
blank line deletion here does not seem to be relevant to the other changes in this commit.
| # eigenvalue at DC | ||
| # TODO: should this be of shape (outputs,inputs) ? | ||
| # TODO: inf rather than nan? | ||
| # TODO: could one not do a bit better, maybe by finding a least squares solution? |
There was a problem hiding this comment.
I think that these questions are good for discussion, but doing so in an issue or on the mailing list would be better than new TODO comments here.
There was a problem hiding this comment.
Fair enough. On reflection, I think the shape remark is correct, and the other two require discussion. Shall I change this to np.tile(np.nan,(self.outputs,self.inputs)) (with unit test), and open an issue on whether we can do better? FWIW, after a bit of thought and some experimentation, I'm not sure how to do better than evaluating DC gain on a minrealed version of each input-output pair.
For discrete time systems the DC gain is found at z=1; change dcgain method in TransferFunction and StateSpace classes for this. Added tests for static gain, low-pass filter, differencer and summer for both classes. If the StateSpace DC gain computation fails due to singularity, return an array of NaNs of size (output,input), rather than scalar NaN. Added separate test for this for continuous- and discrete-time cases.
84033fe to
310f580
Compare
|
Changes Unknown when pulling 310f580 on roryyorke:rory/discr-time-dcgain-fix into ** on python-control:master**. |
|
It seems that Coveralls is still broken. I measured change in test coverage on Python 3.5.2. Results:
as reported from |
| self.C.dot(np.linalg.solve(self.A, self.B))) | ||
| if self.isctime(): | ||
| gain = np.asarray(self.D - | ||
| self.C.dot(np.linalg.solve(self.A, self.B))) |
There was a problem hiding this comment.
Indentation is off alignment here. This is a small matter of style; OK for someone to fix it directly on master branch if there are not other issues with this PR.
|
Your proposed change This also constitutes API change, in particular change to the return type in certain cases. I am not aware of existing applications that would break. Furthermore, NaN spreads readily in a computation. Therefore, I think that once my comment about documentation is addressed, this PR is good to go. |
|
I had planned to fix the StateSpace.dcgain docstring and the whitespace - I guess it's too late on this PR? |
|
I moved forward with the merge because my remaining comments can be addressed separately with no more effort than addressing them here (in this PR) would require. Also I noticed that there are several other issues of indentation style in the statesp.py, so the issue here and in the other locations can be handled separately soon. |
|
I updated the docstring in ee75d9a on |
Fix for #114.
An alternative design would be to have
LTI.dcgaincall eitherself(0)(ctime) orself(1)(dtime);TransferFunction.__call__could special case for s=0, so thatg(0)andg.dcgain(0)return the same thing. In this case__call__would need to be implemented forStateSpace. In my opinionevalfrandfreqrespshould both use__call__, and thathornershould be deprecated.