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 — Nyquist plot improvements by murrayrm · Pull Request #534 · python-control/python-control

Conversation

@murrayrm

@murrayrm murrayrm commented Feb 6, 2021

Copy link
Copy Markdown
Member

This PR provides improvements to Nyquist plots:

  • The nyquist_plot command now returns the number of encirclements of the -1 point and (optionally) the contour used to generate the plot instead of the magnitude, phase, and frequency values. (The system response can be generated from the contour by simply evaluating the LTI system along the contour).
  • Nyquist plots now automatically create a small indentation around poles on the imaginary access, along Nyquist plots to be closed curves. Options are available to change the size of the indentation (indent_radius) and the direction of the indentation for the case of poles exactly on the imaginary axis (set indent_direction to 'left' or 'right'). Indentation can be turned off by setting indent_direction to none).
  • Arrows are now generated using FancyArrowPatch, which provides a fixed size and aspect ratio in display coordinates (so zooming in doesn't not change the size and shape of arrows). The default arrow style can be overridden using the option arrow_size parameter or the arrow_style parameters.
  • The number (or specific location) of arrows can be specified using the arrows keyword, which allows you to specify the number of arrows or the specific locations (via a list of relative positions on the primary segment).
  • The line style for mirror image of the primary segment can be set using the mirror_style parameter and defaults to a dashed line. The mirror image segment can be omitted by setting mirror_style=False.
  • The MATLAB version of nyquist retains its original call signature and return values.

Before:

After:

Other notes:

  • The mirror_style parameter will conflict with the mirror parameter in PR Describing functions #521 (describing functions). The functionality is basically the same, but there will be a merge conflict in whichever gets merged second. (I think we should use mirror_style).
  • The various default settings can now be set using config.defaults with module name nyquist.
  • The control/tests/nyquist_test.py unit test file serves as a pretty complete unit test, but can also be run in python/ipython to generate plots that you can see (and make sure they look OK).
  • I pulled the code for processing arguments for the MATLAB version of bode into a separate function (_parse_freqplot_args) that is used for both bode and nyquist.
  • Added unit tests, docstrings, some PEP8 cleanup.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@coveralls

coveralls commented Feb 6, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.5%) to 88.798% when pulling e0521b9 on murrayrm:improve_nyquist into eb62d80 on python-control:master.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sawyerbfuller
sawyerbfuller reviewed Feb 7, 2021
View reviewed changes
Comment thread control/freqplot.py Outdated

# If argument was a singleton, turn it into a list
if not hasattr(syslist, '__iter__'):
isscalar = not hasattr(syslist, '__iter__')

@sawyerbfuller sawyerbfuller Feb 7, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

IsScalar sounds like it could be referring to the system. ‘Is_Single_system’?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller reviewed Feb 7, 2021
View reviewed changes
Comment thread control/matlab/wrappers.py Outdated
from ..freqplot import nyquist_plot

# If first argument is a list, assume python-control calling format
if (getattr(args[0], '__iter__', False)):

@sawyerbfuller sawyerbfuller Feb 7, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I have seen this style elsewhere in the library so I am curious: isn’t hasattr(args[0], ‘ iter’) both more readable and compact? (I can’t do backticks from my phone so underscores appear as bold)

Sorry, something went wrong.

Uh oh!

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

@bnavigator bnavigator Feb 7, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

hasattr also returns True if args[0] is an iterable sequence, but empty. This code only branches if args[0] is iterable and the sequence does not cast to False (is empty).

Sorry, something went wrong.

Uh oh!

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

@sawyerbfuller sawyerbfuller Feb 8, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I see, thanks!

Sorry, something went wrong.

Uh oh!

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

@murrayrm murrayrm Feb 8, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

getattr([], '__iter__', False) returns <method-wrapper '__iter__' of list object .... So looks like both of them would follow the branch?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller approved these changes Feb 7, 2021
View reviewed changes
@sawyerbfuller

sawyerbfuller commented Feb 7, 2021

Copy link
Copy Markdown
Contributor

Very nice! Plots look great and I think the new return values make sense.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnavigator
bnavigator reviewed Feb 7, 2021
View reviewed changes
Comment thread control/tests/nyquist_test.py Outdated
Comment on lines +37 to +41
@pytest.fixture(scope="function")
def figure_cleanup():
plt.close('all')
yield
plt.close('all')

@bnavigator bnavigator Feb 7, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Did you try to use the existing mplcleanup (automatically loaded from conftest.py)?

python-control/control/tests/conftest.py

Lines 111 to 124 in d75c395

@pytest.fixture(scope="function")
def mplcleanup():
"""Workaround for python2
python 2 does not like to mix the original mpl decorator with pytest
fixtures. So we roll our own.
"""
save = mpl.units.registry.copy()
try:
yield
finally:
mpl.units.registry.clear()
mpl.units.registry.update(save)
mpl.pyplot.close("all")

Now that we don't test python2 anymore, we could directly map it to matplotlib.testing.decorators.cleanup

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@murrayrm
murrayrm force-pushed the improve_nyquist branch from d311109 to dda3149 Compare February 8, 2021 01:55
@bnavigator

bnavigator commented Feb 8, 2021

Copy link
Copy Markdown
Contributor

Hmm, I wonder why the nichols_test xlim warning is back here. The test is run before nyquist test and already uses the mplcleanup fixture.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
murrayrm added 8 commits February 10, 2021 22:02
@murrayrm
add mirror_style keyword for nyquist
85faffa
@murrayrm
update arrow styles for nyquist
1888854
@murrayrm
update MATLAB interface to bode, nyquist
6de5e91 
* regularize argument parsing by using _parse_freqplot_args
* fix bug in exception handling (ControlArgument not defined)
* change printed warning to use warnings.warn
* add unit tests for exceptions, warnings
@murrayrm
improvements to Nyquist contour tracing
2021a75 
* start tracing from omega = 0
* add support for indenting around imaginary poles
* change return value to number of encirclements (+ contour, if desired)
* update MATLAB interface to retain legacy format
* new unit tests
@murrayrm
updated examples and small tweaks to docs
b859aec
@murrayrm
update descfcn module for compatibility
1d9fc80
@murrayrm
small fixes based on @sawyerbfuller, @bnavigator comments
078e028
@murrayrm
add mplcleanup to fix nichols error
e0521b9
@murrayrm
murrayrm force-pushed the improve_nyquist branch from dda3149 to e0521b9 Compare February 11, 2021 06:09
@murrayrm murrayrm mentioned this pull request Feb 11, 2021
arrows in nyquist plot #303
Closed
@murrayrm murrayrm linked an issue Feb 11, 2021 that may be closed by this pull request
arrows in nyquist plot #303
Closed
@murrayrm murrayrm mentioned this pull request Feb 11, 2021
Bode, Nyquist, and root locus plots are crude #49
Open
@murrayrm
murrayrm merged commit 9c6c619 into python-control:master Feb 12, 2021
@murrayrm
murrayrm deleted the improve_nyquist branch March 3, 2021 02:56
@murrayrm murrayrm added this to the 0.9.0 milestone Mar 20, 2021
@murrayrm murrayrm mentioned this pull request Apr 15, 2022
Bug fix and improvements to Nyquist plots #722
Merged
@murrayrm murrayrm mentioned this pull request Aug 19, 2023
Update frequency response plots to use _response/_plot pattern #924
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnavigator bnavigator bnavigator left review comments
@sawyerbfuller sawyerbfuller sawyerbfuller 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.0

Development

Successfully merging this pull request may close these issues.

arrows in nyquist plot

Uh oh!

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

4 participants

@murrayrm @coveralls @sawyerbfuller @bnavigator
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.