BugFix: DC gain for discrete-time systems by roryyorke · Pull Request #126

BugFix: DC gain for discrete-time systems by roryyorke · Pull Request #126 · python-control/python-control

Fix for #114.

An alternative design would be to have LTI.dcgain call either self(0) (ctime) or self(1) (dtime); TransferFunction.__call__ could special case for s=0, so that g(0) and g.dcgain(0) return the same thing. In this case __call__ would need to be implemented for StateSpace. In my opinion evalfr and freqresp should both use __call__, and that horner should be deprecated.

Changes Unknown when pulling 3997637 on roryyorke:rory/discr-time-dcgain-fix into ** on python-control:master**.

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.
Similar behavior is reported in lemurheavy/coveralls-public#351

		gain = np.nan
		return np.squeeze(gain)

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

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:

Before your patch: 76.74% (687 missed statements; 2954 total)
After your patch: 77.47% (667 missed statements; 2960 total)

as reported from

nosetests --with-coverage --cover-html --cover-package=control

		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)))

Choose a reason for hiding this comment

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

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 StateSpace.dcgain to return a matrix of numpy.nan values that has the same shape as the gain matrix is good. The documentation of StateSpace.dcgain should be updated for this.

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 master branch.

roryyorke deleted the rory/discr-time-dcgain-fix branch

January 6, 2017 19:27