- Notifications
You must be signed in to change notification settings - Fork441
recalculate loci for sisotool and rlocus on axis scaling#1153
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedJun 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Pull Request Overview
This PR enables dynamic recalculation of root locus and pole-zero plots when the user pans or zooms insisotool andrlocus, fixing issue#1151.
- Integrate
add_loci_recalculatecallback intosisotoolandroot_locus_plotfor auto-replot on axis changes - Extend
root_locus_mapsignature to acceptxlim,ylimand pass them through to gain calculation - Add
replotsupport for pole-zero maps via a newpole_zero_replotfunction
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| control/sisotool.py | Imported and invokedadd_loci_recalculate, updated axis reference for root locus |
| control/rlocus.py | Addedxlim/ylim parameters, definedadd_loci_recalculate, wired callback inroot_locus_plot |
| control/pzmap.py | Addedreplot method andpole_zero_replot for pole-zero redraw on zoom/pan |
Comments suppressed due to low confidence (2)
control/sisotool.py:141
- Variable
axesis undefined here; you likely meant to usefig.axesor the local axes variable instead ofaxes.
ax_rlocus = axes[0,1] #fig.axes[1]control/pzmap.py:552
ControlPlotinstances expose their axes ascp.axes, notcp.ax. Update tocp.axes[0,0]to avoidAttributeError.
cp.lines[idx,2].append(cp.ax[0,0].plot(| if event.inaxes == ax.axes: | ||
| fig = ax.figure | ||
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.
[nitpick] The original toolbar mode check for'zoom rect' and'pan/zoom' was removed, which may trigger click events during zooming or panning. Consider re-adding that guard to prevent unintended callbacks.
| ifevent.inaxes==ax.axes: | |
| fig=ax.figure | |
| fig=ax.figure | |
| toolbar=fig.canvas.toolbar | |
| iftoolbarisnotNoneandtoolbar.modein ['zoom rect','pan/zoom']: | |
| return | |
| ifevent.inaxes==ax.axes: |
Copilot uses AI. Check for mistakes.
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.
In my experience, the risk of interpreting a click as a valid gain selection is small, and in my testing (with a print statement indicating when the callback is invoked), the zoom and pan events did not propagate to this callback. Removing the zoom_rect and pan/zoom check also removes unintended mode change (updating of the remaining 3 plots is inhibited when zoom / pan is selected), which can be confusing to users.
| # Add a reaction to axis scale changes, if given LTI systems, and | ||
| # there is no set of pre-defined gains | ||
| if gains is None: | ||
| add_loci_recalculate(sysdata, cplt, cplt.axes[0,0]) |
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.
[nitpick] Recalculating the entire locus on everyxlim/ylim change can be expensive. Consider debouncing these callbacks or limiting the update frequency during continuous zooming.
Copilot uses AI. Check for mistakes.
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.
This was no issue when interacting with the root-locus plot. Default number of points calculate is a measly 50, with the adaptive code in_default_gains typically a maximum between 100 and 200 points was produced. The callback will not be activated when the user manually specifies the gains.
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 need to do a bit of testing, but here are some code style comments, mainly in docstrings (see online style guide, linked below).
control/pzmap.py Outdated
| Parameters | ||
| ---------- | ||
| cplt: ControlPlot | ||
| graphics handles of the existing plot |
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.
For consistency with python-control docstring standards, this should start with a capital letter and end in a period. Seehttps://python-control.readthedocs.io/en/latest/develop.html#documentation-guidelines
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.
amended commit
control/pzmap.py Outdated
| @@ -513,6 +524,35 @@ def _click_dispatcher(event): | |||
| return ControlPlot(out, ax, fig, legend=legend) | |||
| def pole_zero_replot(pzmap_responses, cp): | |||
| """Update the loci of a plot after zooming/panning | |||
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.
Should end in a period.
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.
amended commit
control/pzmap.py Outdated
| ---------- | ||
| pzmap_responses : PoleZeroMap list | ||
| Responses to update | ||
| cp : ControlPlot |
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.
Is there a reason to usecp here instead ofcplt, as above?
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.
amended commit
control/rlocus.py Outdated
| # Legacy processing: return locations of poles and zeros as a tuple | ||
| if plot is True: | ||
| return responses.loci, responses.gains | ||
| return ControlPlot(cplt.lines, cplt.axes, cplt.figure) | ||
| def add_loci_recalculate(sysdata, cplt, axis): | ||
| """ Add a calback to re-calculate the loci data fitting a zoom action |
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.
Fix typo (callback) and end in period.
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.
amended commit
control/rlocus.py Outdated
| @@ -46,6 +46,10 @@ def root_locus_map(sysdata, gains=None): | |||
| gains : array_like, optional | |||
| Gains to use in computing plot of closed-loop poles. If not given, | |||
| gains are chosen to include the main features of the root locus map. | |||
| xlim : array_like, 2 elements, optional | |||
| Plot range | |||
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.
Add period. Perhaps also expand description (see other uses of xlim, for example incontrol.root_locus_plot.
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.
amended commit
control/sisotool.py Outdated
| @@ -137,15 +138,18 @@ def sisotool(sys, initial_gain=None, xlim_rlocus=None, ylim_rlocus=None, | |||
| # sys[0, 0], initial_gain=initial_gain, xlim=xlim_rlocus, | |||
| # ylim=ylim_rlocus, plotstr=plotstr_rlocus, grid=rlocus_grid, | |||
| # ax=fig.axes[1]) | |||
| ax_rlocus = fig.axes[1] | |||
| root_locus_map(sys[0, 0]).plot( | |||
| ax_rlocus = axes[0,1] #fig.axes[1] | |||
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.
PEP8: two spaces before #, one space after.
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.
amended commit
34c6d59 intopython-control:mainUh oh!
There was an error while loading.Please reload this page.
adds recalculate of loci for pan/zoom for the root_locus_map and sisotool .Fixes#1151