Skip to content

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

Merged
murrayrm merged 2 commits into
python-control:masterfrom
rabraker:fresp_tb05ad
Jan 6, 2018
Merged

Use slycot's tb05ad for faster and more accurate frequency response #173
murrayrm merged 2 commits into
python-control:masterfrom
rabraker:fresp_tb05ad

Conversation

@rabraker

@rabraker rabraker commented Jan 2, 2018

Copy link
Copy Markdown
Contributor

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

coveralls commented Jan 2, 2018

Copy link
Copy Markdown

Coverage Status

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

@murrayrm murrayrm left a comment

Copy link
Copy Markdown
Member

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.

Comment thread control/statesp.py Outdated
# 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
Copy Markdown
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.

Comment thread control/statesp.py Outdated

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change to omega?

Comment thread control/statesp.py

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

Copy link
Copy Markdown
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
Copy Markdown
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

coveralls commented Jan 4, 2018

Copy link
Copy Markdown

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