Nyquist plot improvements #534

murrayrm · 2021-02-06T20:50:32Z

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.

coveralls · 2021-02-06T20:54:35Z

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

sawyerbfuller · 2021-02-07T05:55:41Z

control/freqplot.py

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


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

sawyerbfuller · 2021-02-07T06:06:51Z

control/matlab/wrappers.py

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


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)

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

I see, thanks!

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

sawyerbfuller · 2021-02-07T06:12:35Z

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

bnavigator · 2021-02-07T11:28:15Z

control/tests/nyquist_test.py

+@pytest.fixture(scope="function")
+def figure_cleanup():
+    plt.close('all')
+    yield
+    plt.close('all')


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

bnavigator · 2021-02-08T10:25:29Z

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.

* 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

* 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

sawyerbfuller reviewed Feb 7, 2021

View reviewed changes

sawyerbfuller approved these changes Feb 7, 2021

View reviewed changes

bnavigator reviewed Feb 7, 2021

View reviewed changes

murrayrm force-pushed the improve_nyquist branch from d311109 to dda3149 Compare February 8, 2021 01:55

murrayrm added 8 commits February 10, 2021 22:02

add mirror_style keyword for nyquist

85faffa

update arrow styles for nyquist

1888854

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

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

updated examples and small tweaks to docs

b859aec

update descfcn module for compatibility

1d9fc80

small fixes based on @sawyerbfuller, @bnavigator comments

078e028

add mplcleanup to fix nichols error

e0521b9

murrayrm force-pushed the improve_nyquist branch from dda3149 to e0521b9 Compare February 11, 2021 06:09

murrayrm mentioned this pull request Feb 11, 2021

arrows in nyquist plot #303

Closed

murrayrm linked an issue Feb 11, 2021 that may be closed by this pull request

arrows in nyquist plot #303

Closed

murrayrm mentioned this pull request Feb 11, 2021

Bode, Nyquist, and root locus plots are crude #49

Open

murrayrm merged commit 9c6c619 into python-control:master Feb 12, 2021

murrayrm deleted the improve_nyquist branch March 3, 2021 02:56

murrayrm added this to the 0.9.0 milestone Mar 20, 2021

murrayrm mentioned this pull request Apr 15, 2022

Bug fix and improvements to Nyquist plots #722

Merged

murrayrm mentioned this pull request Aug 19, 2023

Update frequency response plots to use _response/_plot pattern #924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nyquist plot improvements #534

Nyquist plot improvements #534

murrayrm commented Feb 6, 2021

coveralls commented Feb 6, 2021 •

edited

Loading

sawyerbfuller Feb 7, 2021

sawyerbfuller Feb 7, 2021 •

edited

Loading

bnavigator Feb 7, 2021 •

edited

Loading

sawyerbfuller Feb 8, 2021

murrayrm Feb 8, 2021

sawyerbfuller commented Feb 7, 2021

bnavigator Feb 7, 2021

bnavigator commented Feb 8, 2021

	@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")

Nyquist plot improvements #534

Nyquist plot improvements #534

Conversation

murrayrm commented Feb 6, 2021

coveralls commented Feb 6, 2021 • edited Loading

sawyerbfuller Feb 7, 2021

Choose a reason for hiding this comment

sawyerbfuller Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

bnavigator Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

sawyerbfuller Feb 8, 2021

Choose a reason for hiding this comment

murrayrm Feb 8, 2021

Choose a reason for hiding this comment

sawyerbfuller commented Feb 7, 2021

bnavigator Feb 7, 2021

Choose a reason for hiding this comment

bnavigator commented Feb 8, 2021

coveralls commented Feb 6, 2021 •

edited

Loading

sawyerbfuller Feb 7, 2021 •

edited

Loading

bnavigator Feb 7, 2021 •

edited

Loading