recalculate loci for sisotool and rlocus on axis scaling #1153
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables dynamic recalculation of root locus and pole-zero plots when the user pans or zooms in sisotool and rlocus, 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 invoked add_loci_recalculate, updated axis reference for root locus |
| control/rlocus.py | Added xlim/ylim parameters, defined add_loci_recalculate, wired callback in root_locus_plot |
| control/pzmap.py | Added replot method and pole_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.
[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.
| if event.inaxes == ax.axes: | |
| fig = ax.figure | |
| fig = ax.figure | |
| toolbar = fig.canvas.toolbar | |
| if toolbar is not None and toolbar.mode in ['zoom rect', 'pan/zoom']: | |
| return | |
| if event.inaxes == ax.axes: |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
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 di 10000 d 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.
[nitpick] Recalculating the entire locus on every xlim/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.
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.
control/pzmap.py
Outdated
| Parameters | ||
| ---------- | ||
| cplt: ControlPlot | ||
| graphics handles of the existing plot |
There was a problem hiding this comment.
For consistency with python-control docstring standards, this should start with a capital letter and end in a period. See https://python-control.readthedocs.io/en/latest/develop.html#documentation-guidelines
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 | |||
control/pzmap.py
Outdated
| ---------- | ||
| pzmap_responses : PoleZeroMap list | ||
| Responses to update | ||
| cp : ControlPlot |
There was a problem hiding this comment.
Is there a reason to use cp here instead of cplt, as above?
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.
Fix typo (callback) and end in period.
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.
Add period. Perhaps also expand description (see other uses of xlim, for example in control.root_locus_plot.
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.
PEP8: two spaces before #, one space after.
adds recalculate of loci for pan/zoom for the root_locus_map and sisotool . Fixes #1151