GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — recalculate loci for sisotool and rlocus on axis scaling by repagh · Pull Request #1153 · python-control/python-control

Conversation

repagh commented Jun 5, 2025

Copy link

Copy Markdown

Member

adds recalculate of loci for pan/zoom for the root_locus_map and sisotool . Fixes #1151

Sorry, something went wrong.

Uh oh!

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

All reactions

repagh force-pushed the zoom-rlocus branch from 7d567d6 to 66a3593 Compare

June 5, 2025 21:52

coveralls commented Jun 5, 2025 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

coverage: 94.759% (+0.01%) from 94.746%
when pulling b588045 on repagh:zoom-rlocus
into 632391c on python-control:main.

All reactions

Sorry, something went wrong.

Uh oh!

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

repagh requested a review from Copilot

June 6, 2025 06:50

Copilot AI reviewed

Jun 6, 2025

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

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 in sisotool and rlocus, fixing issue #1151.

Integrate add_loci_recalculate callback into sisotool and root_locus_plot for auto-replot on axis changes
Extend root_locus_map signature to accept xlim, ylim and pass them through to gain calculation
Add replot support for pole-zero maps via a new pole_zero_replot function

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 axes is undefined here; you likely meant to use fig.axes or the local axes variable instead of axes.

ax_rlocus = axes[0,1] #fig.axes[1]

control/pzmap.py:552

ControlPlot instances expose their axes as cp.axes, not cp.ax. Update to cp.axes[0,0] to avoid AttributeError.

cp.lines[idx,2].append(cp.ax[0,0].plot(

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/sisotool.py

Comment on lines +162 to 165

		if event.inaxes == ax.axes:

		fig = ax.figure

Copilot AI Jun 6, 2025

Copy link

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.

Suggested change

	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:

Sorry, something went wrong.

Uh oh!

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

All reactions

Copilot uses AI. Check for mistakes.

repagh Jun 6, 2025

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/rlocus.py

		# 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])

Copilot AI Jun 6, 2025

Copy link

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 every xlim/ylim change can be expensive. Consider debouncing these callbacks or limiting the update frequency during continuous zooming.

Sorry, something went wrong.

Uh oh!

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

All reactions

Copilot uses AI. Check for mistakes.

repagh Jun 6, 2025

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

murrayrm reviewed

Jun 13, 2025

View reviewed changes

murrayrm left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/pzmap.py Outdated

		Parameters
		----------
		cplt: ControlPlot
		graphics handles of the existing plot

murrayrm Jun 13, 2025

Copy link

Copy Markdown

Member

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. See https://python-control.readthedocs.io/en/latest/develop.html#documentation-guidelines

Sorry, something went wrong.

Uh oh!

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

All reactions

repagh Jun 18, 2025

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.

amended commit

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/pzmap.py Outdated



		def pole_zero_replot(pzmap_responses, cp):
		"""Update the loci of a plot after zooming/panning

murrayrm Jun 13, 2025

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

repagh Jun 18, 2025

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.

amended commit

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/pzmap.py Outdated

		----------
		pzmap_responses : PoleZeroMap list
		Responses to update
		cp : ControlPlot

murrayrm Jun 13, 2025

Copy link

Copy Markdown

Member

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 use cp here instead of cplt, as above?

Sorry, something went wrong.

Uh oh!

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

All reactions

repagh Jun 18, 2025

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.

amended commit

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/rlocus.py Outdated



		def add_loci_recalculate(sysdata, cplt, axis):
		""" Add a calback to re-calculate the loci data fitting a zoom action

murrayrm Jun 13, 2025

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

repagh Jun 18, 2025

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.

amended commit

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/rlocus.py Outdated

		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

murrayrm Jun 13, 2025

Copy link

Copy Markdown

Member

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 in control.root_locus_plot.

Sorry, something went wrong.

Uh oh!

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

All reactions

repagh Jun 18, 2025

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.

amended commit

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

control/sisotool.py Outdated

		# ax=fig.axes[1])
		ax_rlocus = fig.axes[1]
		root_locus_map(sys[0, 0]).plot(
		ax_rlocus = axes[0,1] #fig.axes[1]

murrayrm Jun 13, 2025

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

repagh Jun 18, 2025

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.

amended commit

Sorry, something went wrong.

Uh oh!

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

All reactions

repagh force-pushed the zoom-rlocus branch from 66a3593 to 2877443 Compare

June 18, 2025 07:12


          recalculate loci for sisotool and rlocus on axis scaling

b588045

repagh force-pushed the zoom-rlocus branch from 2877443 to b588045 Compare

June 19, 2025 11:11

murrayrm merged commit 34c6d59 into python-control:main

Jun 21, 2025

murrayrm added this to the 0.10.2 milestone

Jul 4, 2025

Reviewers

murrayrm

murrayrm left review comments

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.10.2

Development

Successfully merging this pull request may close these issues.

Regression: root-locus plots may be coarse, and no longer react to zoom events

Uh oh!

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

4 participants

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 01402520 — 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.

Conversation

repagh commented Jun 5, 2025

Uh oh!

coveralls commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jun 5, 2025 •

edited

Loading

Uh oh!

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