Skip to content

Use slycot's tb05ad for faster and more accurate frequency response #173

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 6, 2018

Conversation

rabraker
Copy link
Contributor

@rabraker rabraker commented Jan 2, 2018

This pull request addresses issue #116.

It uses the (relatively) newly wrapped TB05AD function from Slycot, when Slycot is available. If slycot is not available, I instead use the state space built-in method horner, rather than converting to a transfer function for the evaluation.

…valuation of state-space systems. If slycot is unavailable, use the built in horner function (instead of converting to a transfer function, as was done before).
@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage increased (+0.02%) to 78.525% when pulling 138c097 on rabraker:fresp_tb05ad into 33bebc1 on python-control:master.

Copy link
Member

@murrayrm murrayrm left a comment

Choose a reason for hiding this comment

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

Overall, this looks OK. A couple of minor comments that could use a response.

@@ -386,22 +386,98 @@ def horner(self, s):
return array(resp)

# Method for generating the frequency response of the system
def freqresp(self, omega):
"""Evaluate the system's transfer func. at a list of ang. frequencies.
def freqresp(self, omega_s):
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use omega_s here instead of omega? In all other docstrings for freqresp (eg, xferfcn.py, lti.py) we just use omega. Also, shows up as omega below. I would leave as omega for consistency.


Inputs:
------
omega_s: A list of frequencies in radians/sec at which the system
Copy link
Member

Choose a reason for hiding this comment

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

Change to omega?


numFreqs = len(omega_s)
Gfrf = np.empty((self.outputs, self.inputs, numFreqs),
dtype=np.complex128)
Copy link
Member

Choose a reason for hiding this comment

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

Why complex128? I don't have a specific objection, but seems a bit arbitrary. OK to leave, unless there is some justification for doing something else (perhaps just `complex', which presumably defaults to machine precision?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I did this because the numpy page about dtypes doesn't list complex as a type.

Looking further down the page though, they say that dtype=complex will be interpreted as dtype=np.complex_ which is shorthand for np.complex128. To me, using np.complex128 is the most explicit as to what's happening.

@coveralls
Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage increased (+0.04%) to 78.546% when pulling 62a29d9 on rabraker:fresp_tb05ad into 33bebc1 on python-control:master.

@murrayrm murrayrm merged commit 7b2defe into python-control:master Jan 6, 2018
@rabraker rabraker deleted the fresp_tb05ad branch January 6, 2018 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants