Fix s-grid rendering on Matplotlib 3.11 by disabling longitude unwrapping - #1250
aduan5567-netizen wants to merge 3 commits into
Conversation
slivingston
left a comment
There was a problem hiding this comment.
Is there a solution that does not involve duplicating lots of code?
|
Thanks — I refactored the shared sampling, unwrapping, padding, and clipping logic into |
| # 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'): |
There was a problem hiding this comment.
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_bboxis 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
ExtremeFinderCyclethat overrides__call__?
Concerns 2 and 3 require more investigation. Hopefully you can do it :)
There was a problem hiding this comment.
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.
|
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 :) ) |
Visual comparison
I rendered the same root-locus plot (
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 |
|
@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. |
|
@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). |
Refs #1147.
Rationale
The discussion in #1147 covers two version-dependent symptoms of the
omega-damping (s-plane) grid on root-locus plots:
the plot — the original report. This PR does not change that behavior;
it is a separate problem and is left untouched here.
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.pyrenders the s-plane damping-ratio / natural-frequencygrid for root-locus and pole-zero diagrams using axisartist's
GridHelperCurveLinear. The grid only covers the left half-plane, a fixedlongitude interval of 90°–270°. To keep that interval from being unwrapped
at the 180° boundary,
ModifiedExtremeFinderCyclesubclassedExtremeFinderCycleand overrode__call__to apply a 360° unwrappingthreshold 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 onlya 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:lon_cycle=Nonedisables the periodic longitude unwrappingaltogether, so the 90°–270° interval can never be wrapped across the
180° boundary.
lon_minmax=(90, 270)setting still clips the computedgrid 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
ModifiedExtremeFinderCyclefromcontrol/grid.py.sgrid(), construct the stockangle_helper.ExtremeFinderCyclewith
lon_cycle=None(instead of the custom subclass withlon_cycle=360);lat_cycle=None,lon_minmax=(90, 270), andlat_minmax=(0, np.inf)are unchanged.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 identitytransform 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 onboth Matplotlib versions.
The removed class is internal to
control/grid.py; there are no publicAPI changes.
Testing
grid_test.pyandrlocus_test.py(grid and root-locus): 80passed, 1 skipped;
pzmap_test.pyandctrlplot_test.py(pole-zero and controlplots): 126 passed, 2 skipped;
grid_test.pyandrlocus_test.py— 80passed, 1 skipped; the legacy behavior is unchanged.
ruff checkandgit diff --check; both are clean.AI-assisted development disclosure
I used AI tools while preparing this patch:
ModifiedExtremeFinderCycle, switchingsgrid()to the stockExtremeFinderCyclewithlon_cycle=None, and addingcontrol/tests/grid_test.py.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=Noneprevents the 180° unwrapping, howlon_minmaxkeeps thegrid within the left half-plane, and the test's expected
(90, 270, 0, 1.05)bounds (the1.05comes from the bbox padding withny = 20, clamped bylat_minmax). This PR description was written byme. I take full responsibility for the correctness of this patch.