GeezerVison

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

GeezerVison is showing github.com — Fix s-grid rendering on Matplotlib 3.11 by disabling longitude unwrapping by aduan5567-netizen · Pull Request #1250 · python-control/python-control

Conversation

aduan5567-netizen commented Sep 19, 2026 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Refs #1147.

Rationale

The discussion in #1147 covers two version-dependent symptoms of the
omega-damping (s-plane) grid on root-locus plots:

With Matplotlib 3.10, enabling the grid adds extra whitespace around
the plot — the original report. This PR does not change that behavior;
it is a separate problem and is left untouched here.
With Matplotlib 3.11, the damping grid lines themselves render
incorrectly. As diagnosed in the issue discussion, this is a regression
caused by matplotlib's axisartist refactor, and it is what this PR
fixes.

control/grid.py renders the s-plane damping-ratio / natural-frequency
grid for root-locus and pole-zero diagrams using axisartist's
GridHelperCurveLinear. The grid only covers the left half-plane, a fixed
longitude interval of 90°–270°. To keep that interval from being unwrapped
at the 180° boundary, ModifiedExtremeFinderCycle subclassed
ExtremeFinderCycle and overrode __call__ to apply a 360° unwrapping
threshold instead of the upstream 180°.

Matplotlib 3.11 moved the sampling/bounding-box logic into a new
ExtremeFinderSimple._find_transformed_bbox(transform, bbox) method,
which GridFinder.get_grid_info() now calls directly; __call__ is only
a thin backward-compatible wrapper. Our override is therefore bypassed on
3.11, the upstream 180° unwrapping is applied, and the grid extremes are
computed incorrectly.

Rather than porting the override to the new interface, this PR removes
the custom subclass entirely and uses the stock ExtremeFinderCycle:

Setting lon_cycle=None disables the periodic longitude unwrapping
altogether, so the 90°–270° interval can never be wrapped across the
180° boundary.
The existing lon_minmax=(90, 270) setting still clips the computed
grid bbox to the left half-plane, which is what actually enforces the
fixed interval.

This configuration works on both Matplotlib 3.10 and 3.11, and the 3.10
behavior — including the whitespace reported in #1147 — is unchanged.

Changes

Delete ModifiedExtremeFinderCycle from control/grid.py.
In sgrid(), construct the stock angle_helper.ExtremeFinderCycle
with lon_cycle=None (instead of the custom subclass with
lon_cycle=360); lat_cycle=None, lon_minmax=(90, 270), and
lat_minmax=(0, np.inf) are unchanged.
Add control/tests/grid_test.py::test_sgrid_does_not_unwrap_longitude,
which obtains the extreme finder from the axes built by sgrid(),
asserts finder.lon_cycle is None, and checks that an identity
transform over 90°–270.001° returns extremes (90, 270, 0, 1.05).
The 270.001° edge would trigger the stock 180° unwrapping
(270.001 - 90 > 180), so the test guards against this regression on
both Matplotlib versions.

The removed class is internal to control/grid.py; there are no public
API changes.

Testing

Matplotlib 3.11.2: I ran the relevant tests
- grid_test.py and rlocus_test.py (grid and root-locus): 80
  passed, 1 skipped;
- pzmap_test.py and ctrlplot_test.py (pole-zero and control
  plots): 126 passed, 2 skipped;
- total: 206 passed, 3 skipped.
Matplotlib 3.10.8: I ran grid_test.py and rlocus_test.py — 80
passed, 1 skipped; the legacy behavior is unchanged.
I ran ruff check and git diff --check; both are clean.

AI-assisted development disclosure

I used AI tools while preparing this patch:

OpenAI Codex drafted the code changes: removing
ModifiedExtremeFinderCycle, switching sgrid() to the stock
ExtremeFinderCycle with lon_cycle=None, and adding
control/tests/grid_test.py.
Doubao (ByteDance's AI assistant) assisted with problem analysis:
I used it to trace the Matplotlib 3.11 refactor against the upstream
3.11.2 source, confirming that GridFinder.get_grid_info() now calls
_find_transformed_bbox() directly, which is why the pre-existing
__call__ override was bypassed.

I reviewed every AI-assisted change line by line, and I can explain what
the code does and why it is implemented this way — including why
lon_cycle=None prevents the 180° unwrapping, how lon_minmax keeps the
grid within the left half-plane, and the test's expected
(90, 270, 0, 1.05) bounds (the 1.05 comes from the bbox padding with
ny = 20, clamped by lat_minmax). This PR description was written by
me. I take full responsibility for the correctness of this patch.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Fix s-grid rendering on Matplotlib 3.11

slivingston requested changes

Sep 19, 2026

View reviewed changes

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

Is there a solution that does not involve duplicating lots of code?

Sorry, something went wrong.

Uh oh!

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

All reactions


          Refactor extreme finder compatibility path

329869c

aduan5567-netizen commented Sep 20, 2026

Copy link

Copy Markdown

Author

Thanks — I refactored the shared sampling, unwrapping, padding, and clipping logic into _find_extremes(). The legacy __call__ path now only adapts the callable transform and tuple ordering, while the Matplotlib 3.11 path forwards trans.transform. This removes the duplicated algorithm while preserving compatibility with both versions. I tested it against Matplotlib 3.10.8 and 3.11.2.

All reactions

Sorry, something went wrong.

Uh oh!

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

slivingston reviewed

Sep 20, 2026

View reviewed changes

Comment thread

control/grid.py Outdated

		# Matplotlib 3.11 moved this calculation to
		# _find_transformed_bbox(), which is also called directly by
		# GridFinder. Keep the legacy implementation for older releases.
		if hasattr(super(), '_find_transformed_bbox'):

slivingston Sep 20, 2026

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.

Thanks for refactoring. Now that I am investigating this more, I think this approach overall is not the best. My main concerns:

This code checks for the presence of an intended-as-hidden superclass method (_find_transformed_bbox). Such inspection is bad because _find_transformed_bbox is not part of the public API of Matplotlib. (I might be wrong about this; please correct me if so.). Therefore, this check is fragile and could break unexpectedly.
Could this be a bug in Matplotlib? I doubt it, but the fact that output changed in a surprising way with the new version of Matplotlib suggests it is worth exploring the Matplotlib side more: check issue trackers, changelogs, etc.
Perhaps our design here is wrong and needs to be reconsidered. Is there some way to achieve the same result without making a subclass of ExtremeFinderCycle that overrides __call__ ?

Concerns 2 and 3 require more investigation. Hopefully you can do it :)

Sorry, something went wrong.

Uh oh!

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

All reactions

aduan5567-netizen Sep 20, 2026

Copy link

Copy Markdown

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.

Good point — I agree that overriding _find_transformed_bbox and
branching on hasattr would lean on a private API that was just
reorganized, and would be fragile against future matplotlib releases.

I went through matplotlib/matplotlib#27551 (the refactor that
introduced _find_transformed_bbox and deprecated
GridFinder.transform_xy/inv_transform_xy), the 3.11 API changes
notes, and the matplotlib issue tracker to check whether there is a
supported hook for this — there isn't one. But stepping back, the
custom subclass's only change was the unwrap threshold (180° → 360°).
Since the s-grid longitude always lies within the fixed 90°–270°
interval, that 360° threshold effectively never unwraps anything, so
it is equivalent to disabling cycle unwrapping altogether.

I've therefore removed ModifiedExtremeFinderCycle entirely and now
use the stock ExtremeFinderCycle with lon_cycle=None, relying on
the existing lon_minmax=(90, 270) to clip the grid to the left
half-plane. The new test asserts lon_cycle is None and that an
identity transform over 90–270.001° gives extremes
(90, 270, 0, 1.05) — the 270.001° edge would trigger the stock 180°
unwrapping, so it guards against this regression.

Behavior on 3.10 is unchanged (including the whitespace issue from
#1147, which is out of scope here). The relevant tests pass on both
versions: matplotlib 3.11.2 — 206 passed, 3 skipped (grid, root-locus,
pole-zero, and control-plot tests); matplotlib 3.10.8 — 80 passed,
1 skipped. ruff check and git diff --check are clean.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Remove custom s-grid extreme finder

2a140b1

aduan5567-netizen changed the title ~~Fix s-grid rendering on Matplotlib 3.11 by overriding _find_transformed_bbox~~ Fix s-grid rendering on Matplotlib 3.11 by disabling longitude unwrapping

Sep 20, 2026

aduan5567-netizen requested a review from slivingston

September 20, 2026 07:03

slivingston commented Sep 20, 2026

Copy link

Copy Markdown

Member

To help with review, can you generate before and after images to demonstrate that behavior is unchanged with Matplotlib 3.10 and fixed with Matplotlib 3.11 ? Please show the code used to generate the images.

(This would be a lot of effort if done manually, but you seem to be using AI tools for most/all of this, so my request seems reasonable :) )

All reactions

Sorry, something went wrong.

Uh oh!

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

aduan5567-netizen commented Sep 20, 2026

Copy link

Copy Markdown

Author

Visual comparison

	before (`a775479`)	after (`2a140b1`)
Matplotlib 3.10.8
Matplotlib 3.11.2

I rendered the same root-locus plot (ct.tf([1, 2], [1, 2, 3]),
grid=True) at a775479 (before) and 2a140b1 (after) with
Matplotlib 3.10.8 and 3.11.2, using one fixed Agg script (same figure
size, DPI, and rcParams).

3.10.8: the before/after PNGs are byte-for-byte identical — both
have SHA-256 28DDCB09C8C09B99EC1481E9416FA3714684658987BC89774E470F3B4E8C7D8B.
3.11.2: before draws the damping grid only in the upper
half-plane; after renders it symmetrically, matching 3.10.

The images were generated with:

"""Generate a deterministic root-locus image for PR #1250."""

import argparse

import matplotlib

matplotlib.use("Agg")

import matplotlib.pyplot as plt

import control as ct


parser = argparse.ArgumentParser()
parser.add_argument("output")
args = parser.parse_args()

matplotlib.rcParams.update({
    "figure.dpi": 120,
    "font.size": 10,
    "savefig.dpi": 120,
})

system = ct.tf([1, 2], [1, 2, 3])
ct.root_locus_plot(system, grid=True)

figure = plt.gcf()
figure.set_size_inches(6.4, 4.8)
figure.suptitle(f"Matplotlib {matplotlib.__version__}", fontsize=11)
figure.savefig(args.output)
plt.close(figure)

Run python generate_comparison.py OUTPUT.png in each checkout.

All reactions

Sorry, something went wrong.

Uh oh!

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

slivingston commented Sep 29, 2026

Copy link

Copy Markdown

Member

@aduan5567-netizen Regarding the statement in the PR description, "With Matplotlib 3.10, enabling the grid adds extra whitespace around the plot — the original report. This PR does not change that behavior; it is a separate problem and is left untouched here.", can you explain why that problem is not fixed here? Do you know how to fix it? Maybe we can just include both in this PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

slivingston commented Sep 29, 2026

Copy link

Copy Markdown

Member

@aduan5567-netizen Also, can you confirm that there is a human in the loop? Or, are you an AI agent running autonomously (opening PRs, making comments, etc. without a human).

All reactions

Sorry, something went wrong.

Uh oh!

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

Reviewers

slivingston

Awaiting requested review from slivingston

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

2 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 01402503 — 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.

GeezerVison

Conversation

aduan5567-netizen commented Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Changes

Testing

AI-assisted development disclosure

Uh oh!

slivingston left a comment

Choose a reason for hiding this comment

Uh oh!

aduan5567-netizen commented Sep 20, 2026

Uh oh!

slivingston Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

aduan5567-netizen Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

slivingston commented Sep 20, 2026

Uh oh!

aduan5567-netizen commented Sep 20, 2026

Visual comparison

Uh oh!

slivingston commented Sep 29, 2026

Uh oh!

slivingston commented Sep 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aduan5567-netizen commented Sep 19, 2026 •

edited

Loading

Uh oh!

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