GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Round to nearest integer decade for default omega vector by bnavigator · Pull Request #688 · python-control/python-control

Conversation

@bnavigator

@bnavigator bnavigator commented Jan 4, 2022

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bnavigator
round to nearest integer for default omega
58d8cd6
@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

coveralls commented Jan 4, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Coverage Status

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator added 2 commits January 4, 2022 21:07
@bnavigator
split up nyquist indent tests
19801e6
@bnavigator
passthrough Hz parameter for omega vector
c44b901
@bnavigator

bnavigator commented Jan 4, 2022

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator
bnavigator commented Jan 4, 2022
View reviewed changes
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bnavigator
docstring punctuation
021372f
murrayrm
murrayrm approved these changes Jan 5, 2022
View reviewed changes
@bnavigator
bnavigator merged commit df6b352 into python-control:master Jan 5, 2022
@murrayrm

murrayrm commented Jan 5, 2022

Copy link
Copy Markdown
Member

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnavigator

bnavigator commented Jan 5, 2022

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

Reviewers

@murrayrm murrayrm murrayrm approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.2

Development

Successfully merging this pull request may close these issues.

python-control 0.9.1 fails in test_nyquist_indent on aarch64

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@bnavigator @coveralls @murrayrm
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.