Update pole/zero and root locus plots to use _map/_plot pattern#953
Merged
murrayrm merged 16 commits intopython-control:mainfrom Jan 12, 2024
Merged
Update pole/zero and root locus plots to use _map/_plot pattern#953murrayrm merged 16 commits intopython-control:mainfrom
murrayrm merged 16 commits intopython-control:mainfrom
Conversation
7 tasks
Merged
bnavigator
reviewed
Jan 10, 2024
control/sisotool.py
Outdated
Comment on lines
3
to
6
| import numpy as np | ||
| import matplotlib.pyplot as plt | ||
| import warnings | ||
| from functools import partial |
Contributor
There was a problem hiding this comment.
Worth running this through isort?
warnings and functools are stdlib, Numpy and Pyplot external.
bnavigator
reviewed
Jan 10, 2024
control/tests/pzmap_test.py
Outdated
Comment on lines
97
to
106
| # Legacy return format | ||
| with pytest.warns(DeprecationWarning, match=".* values .* deprecated"): | ||
| poles, zeros = ct.pole_zero_plot(pzdata, plot=False) | ||
| np.testing.assert_equal(poles, sys.poles()) | ||
| np.testing.assert_equal(zeros, sys.zeros()) | ||
|
|
||
| with pytest.warns(DeprecationWarning, match=".* values .* deprecated"): | ||
| poles, zeros = ct.pole_zero_plot(pzdata, plot=True) | ||
| np.testing.assert_equal(poles, sys.poles()) | ||
| np.testing.assert_equal(zeros, sys.zeros()) |
bnavigator
approved these changes
Jan 10, 2024
Contributor
bnavigator
left a comment
There was a problem hiding this comment.
This is large, without too extensive review, I would say LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR updates pole/zero and root locus plots to use the _map/_plot calling pattern described in #645. The following patterns will work:
Everything is mostly backwards compatible except that the outputs from a _plot function are now an array of Line2D objects instead of the roots and gains. You can still get the original data using the _map function and there is some legacy processing if you used the
plotkeyword to try let some code work without changes.Summary of changes:
_mapfunction and a_plotfunction. You can access the latter via the.plot()method on the response._plotfunctions accept either the output of the_mapfunction or a list of systems (in which case the_mapfunction is called internally). This allows the common pattern ofpole_zero_plot(sys),root_locus_plot(sys)to work as expected.sisotool). Clicking on a portion of the root locus diagram will generate markers at the locations on the loci corresponding to that gain and add a message above the plot giving the frequency and damping ratio for the point that was clicked.print_gainkeyword is replaced withinteractive.Plotkeyword is nowplot(and generates a warning, since it triggers the legacy return values).pzmapandrlocusare still there.control.matlabversion ofrlocusreturnsroots, gains(compatible with MATLAB)This PR is going to break existing code. It would be great if a few people could try this out so that we can make sure we are OK with the changes here.
Examples (from the user documentation):
Pole/zero maps and root locus diagrams provide insights into system response based on the locations of system poles and zeros in the complex plane. The
pole_zero_map()function returns the poles and zeros and can be used to generate a pole/zero plot:A root locus plot shows the location of the closed loop poles of a system as a function of the loop gain:
The grid in the left hand plane shows lines of constant damping ratio as well as arcs corresponding to the frequency of the complex pole. The grid can be turned off using the
gridkeyword. Setting grid toFalse will turn off the grid but show the real and imaginary axis. To completely remove all lines except the root loci, usegrid=’empty’.On systems that support interactive plots, clicking on a location on the root locus diagram will mark the pole locations on all branches of the diagram and display the gain and damping ratio below the plot title:
Root locus diagrams are also supported for discrete time systems, in which case the grid is show inside the unit circle:
Lists of systems can also be given, in which case the root locus diagram for each system is plotted in different colors: