Skip to content

Round to nearest integer decade for default omega vector #688

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 4 commits into from
Jan 5, 2022

Conversation

bnavigator
Copy link
Contributor

Fixes #687.

The docstring for the _default_frequency_range() function says

This code looks at the poles and zeros of all of the systems that
we are plotting and sets the frequency range to be one decade above
and below the min and max feature frequencies, rounded to the nearest
integer.

The old code rounded to the floor and ceiling of the range, which occasionally adds another decade to one or the other end of the frequency range if the feature falls just inside of the outermost decade.

The new code rounds to the nearest integer, so it only adds half a decade at minimum.

@bnavigator bnavigator changed the title Round to nearest integer for default omega vector Round to nearest integer decade for default omega vector Jan 4, 2022
@coveralls
Copy link

coveralls commented Jan 4, 2022

Coverage Status

Coverage increased (+0.09%) to 93.591% when pulling 021372f on bnavigator:omega_default_round into a111b03 on python-control:master.

@bnavigator
Copy link
Contributor Author

Looking at the coveralls report, it turns out the Hz parameter was never passed through to _default_frequency_range(). Fixed now.

From the sisotool test which had to be changed:

from numpy import array
from matplotlib import pyplot as plt
from control import TransferFunction, bode_plot

tsys = TransferFunction(array([1000]), array([  1,  25, 100,   0]))
# poles: [-20.,  -5.,   0.]

for Hz in [True, False]:
    plt.figure()
    bode_plot(tsys, Hz=Hz)
Hz False True
Before Figure-rad-before Figure-Hz-before
After Figure-rad-after Figure-Hz-after

@bnavigator bnavigator merged commit df6b352 into python-control:master Jan 5, 2022
@murrayrm
Copy link
Member

murrayrm commented Jan 5, 2022

@bnavigator In thinking through this overnight, I wonder if we might want to revert to the old behavior (floor, ceil) and update the documentation to match (+ update the unit tests to avoid the corner case). I'm worried that only going out a half decade might now show the asymptotic frequency response. I'll work up a couple of examples and post an issue so that we can discuss.

@bnavigator
Copy link
Contributor Author

Hmm, okay. We need to find another way to make test_nyquist_indent more robust then.

@murrayrm murrayrm added this to the 0.9.2 milestone Apr 17, 2022
@bnavigator bnavigator deleted the omega_default_round branch February 18, 2024 20:30
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.

python-control 0.9.1 fails in test_nyquist_indent on aarch64
3 participants