Fix contour labels and colorbar semantics [mpl compatibility] - #341
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThis change advances the wire protocol to v9 and adds logarithmic normalization, contour label placement, line-only and exact-color colorbars, explicit colorbar axes, updated layout behavior, cross-renderer support, compatibility documentation, and regression tests. ChangesMatplotlib compatibility and rendering
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Axes as Axes.imshow/pcolormesh
participant Figure as Figure.colorbar
participant Renderer as SVG/Raster/ChartView renderer
Axes->>Figure: provide normalized domain and scale metadata
Figure->>Figure: build colorbar options and placement
Figure->>Renderer: serialize colorbar bands, ticks, overlays, and extensions
Renderer-->>Figure: render colorbar output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
js/src/50_chartview.ts (1)
2539-2571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo identical fraction closures.
colorbarFraction(Line 2543) andfractionFor(Line 2569) are byte-identical; keep one so the log/linear mapping can't drift between line markers and ticks.♻️ Proposed dedupe
- for (const raw of tickValues) { + for (const raw of tickValues) {(remove the second definition and reuse
colorbarFractionat Lines 2579 / 2597)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/src/50_chartview.ts` around lines 2539 - 2571, Remove the duplicate fractionFor closure and reuse colorbarFraction for all subsequent tick and label positioning calculations, including the usages near the indicated later lines. Keep the existing log/linear mapping behavior unchanged.python/xy/_svg.py (1)
3953-4093: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid round-tripping the formatted width back through
float(). Line 4068 stores_num(...)(a formatted string) inline_widthand Lines 4070-4071 re-parse it to compute the dash pattern; keep the numeric value separate from its serialized form.♻️ Proposed tweak
- line_width = _num(max(0.5, float(line.get("width", 1.0)))) + width_value = max(0.5, float(line.get("width", 1.0))) + line_width = _num(width_value) dash = ( - f' stroke-dasharray="{_num(3.7 * float(line_width))} ' - f'{_num(1.6 * float(line_width))}"' + f' stroke-dasharray="{_num(3.7 * width_value)} {_num(1.6 * width_value)}"' if line.get("dash") == "dashed" else "" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/_svg.py` around lines 3953 - 4093, Update the line rendering loop around line_width so the numeric width remains available for dash-pattern calculations while its _num-formatted string is used only for the SVG stroke-width attribute. Compute the dash values from the original numeric width, avoiding any float() conversion of the serialized result; preserve the existing minimum width behavior and output.python/xy/pyplot/_mplfig.py (1)
737-747: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate tick-candidate-selection logic.
This block re-implements the exact same "top-~10 candidates closest to zero" algorithm as the pre-existing
elif levels is not None and entry.get("discrete_boundaries") is not None:branch just below (lines 748-761), differing only in the source array. Extracting a small helper (e.g._select_colorbar_ticks(locations)) would remove the duplication and prevent the two copies from drifting if the heuristic changes later.♻️ Proposed refactor
+def _select_colorbar_ticks(locations: np.ndarray) -> list[float]: + step = max(1, int(np.ceil(len(locations) / 10))) + candidates = [locations[offset::step] for offset in range(step)] + selected = min(candidates, key=lambda values: np.min(np.abs(values))) + zero_tolerance = np.finfo(np.float64).eps * max(1.0, float(np.max(np.abs(locations)))) * 8 + return [0.0 if abs(float(value)) <= zero_tolerance else float(value) for value in selected] + ... if ticks is not None: options["ticks"] = [float(value) for value in np.asarray(ticks).reshape(-1)] elif line_contour: - locations = np.asarray(mappable.levels, dtype=np.float64).reshape(-1) - step = max(1, int(np.ceil(len(locations) / 10))) - candidates = [locations[offset::step] for offset in range(step)] - selected = min(candidates, key=lambda values: np.min(np.abs(values))) - zero_tolerance = ( - np.finfo(np.float64).eps * max(1.0, float(np.max(np.abs(locations)))) * 8 - ) - options["ticks"] = [ - 0.0 if abs(float(value)) <= zero_tolerance else float(value) for value in selected - ] + options["ticks"] = _select_colorbar_ticks( + np.asarray(mappable.levels, dtype=np.float64).reshape(-1) + ) elif levels is not None and entry.get("discrete_boundaries") is not None: - locations = np.asarray(entry["discrete_boundaries"], dtype=np.float64).reshape(-1) - step = max(1, int(np.ceil(len(locations) / 10))) - candidates = [locations[offset::step] for offset in range(step)] - selected = min(candidates, key=lambda values: np.min(np.abs(values))) - zero_tolerance = ( - np.finfo(np.float64).eps * max(1.0, float(np.max(np.abs(locations)))) * 8 - ) - options["ticks"] = [ - 0.0 if abs(float(value)) <= zero_tolerance else float(value) for value in selected - ] + options["ticks"] = _select_colorbar_ticks( + np.asarray(entry["discrete_boundaries"], dtype=np.float64).reshape(-1) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/pyplot/_mplfig.py` around lines 737 - 747, Extract the shared “select up to ~10 colorbar ticks closest to zero” algorithm from the line_contour branch and the following discrete_boundaries branch into a helper such as _select_colorbar_ticks(locations). Update both branches to call the helper with their respective source arrays, preserving the existing sampling, zero-tolerance, and tick-value behavior.python/xy/pyplot/_axes.py (1)
6286-6299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winColorbar chrome constants are now duplicated verbatim in four renderers.
_colorbar_outside_roommirrors the exact numbers in_svg.layout()(44/24/62/86/18/38 + 18/16 label) and the raster/JS equivalents. The mirror is correct today, but nothing fails loudly if one side changes. Consider exporting these from a single module (e.g. alongside_svg.layout) and importing them here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/pyplot/_axes.py` around lines 6286 - 6299, The colorbar sizing constants in _colorbar_outside_room are duplicated across renderers and can drift. Define shared exported constants alongside the existing _svg.layout sizing definitions, then update _colorbar_outside_room and the raster/JS equivalents to import and reuse them while preserving the current orientation, placement, padding, and label calculations.python/xy/pyplot/_colors.py (1)
503-511: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing this
extreme()resolver in_axes.py.
Axes.imshow's localextreme()(python/xy/pyplot/_axes.py lines 2599-2611) resolves onlycmap._{name}and misses the_rgba_{name}fallback added here, so a realmatplotlib.colors.Colormapwithset_bad/set_undertakes a different path in thehas_extremesbranch than inscalar_grid_rgba. Exporting this helper (or its lookup) would remove the divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/pyplot/_colors.py` around lines 503 - 511, The local extreme() resolver in Axes.imshow should reuse the shared resolver from _colors.py so it also handles _rgba_{name} fallbacks and tuple values consistently. Export the existing helper or its lookup, update _axes.py’s has_extremes path to call it, and remove the duplicated local resolution logic while preserving the existing default handling.python/xy/pyplot/_plot_types.py (2)
821-825: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePer-channel interpolation of straight (non-premultiplied) RGBA.
For the log path
gridis truecolor RGBA where bad cells carry alpha 0 but arbitrary RGB. Interpolating R/G/B independently of A bleeds the bad cell's RGB into its neighbours as a faint halo. Premultiplying before the interpolation and dividing back afterwards avoids it. Only reachable forshading="gouraud"with masked/non-positive samples, so low priority.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/pyplot/_plot_types.py` around lines 821 - 825, Update the 3D RGBA branch in _bilinear_grid to premultiply RGB channels by alpha before interpolation, interpolate alpha alongside them, then unpremultiply the resulting RGB using a safe zero-alpha guard. Preserve the existing per-channel stack shape and behavior for non-RGBA inputs.
478-487: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRow-wise Python iteration over the segment array.
for xa, ya, xb, yb in segmentsunboxes every marching-squares segment in Python; on a dense grid with several levels this is the dominant cost ofclabel(). Since the keys are just quantized integers, the whole table can be built vectorized (np.rint(segments / tolerance).astype(np.int64)) and only the adjacency dict built in Python. Optional —clabel()is off the default build path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/pyplot/_plot_types.py` around lines 478 - 487, Update the segment-processing block around segments to quantize all endpoint coordinates vectorized with NumPy rather than unboxing rows through Python iteration. Preserve the existing tolerance-based key behavior and skip degenerate segments where both endpoint keys match, while limiting Python work to constructing endpoints, positions, and adjacency from the resulting key arrays.tests/pyplot/test_gallery_log_colorbar_blockers.py (1)
29-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for
clabel()on an axes that already holds another contour.The suite never places two contour entries on one axes before calling
clabel, which is exactly the state that trips the value-basedself._entries.index(source)lookup flagged inpython/xy/pyplot/_plot_types.pylines 3689-3692. Acontourf+contour+clabelcase would pin it.Want me to draft that test?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/pyplot/test_gallery_log_colorbar_blockers.py` around lines 29 - 211, The test suite needs coverage for labeling a contour when another contour already exists on the same axes. Add a regression test near the existing contourf colorbar test that creates a contourf and then a separate contour on one axes, calls clabel on the latter, and verifies the labels render or are associated with the correct contour, exercising the ContourSet.clabel path and preventing value-based entry lookup failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/xy/pyplot/_colors.py`:
- Around line 503-511: Extract the nested extreme resolver from
scalar_grid_rgba.extreme into an exported module-level helper such as
cmap_extreme(cmap, name, default), preserving support for both _under/_over/_bad
and _rgba_under/_rgba_over/_rgba_bad attributes. Update
python/xy/pyplot/_plot_types.py lines 3150-3151 to use this helper for contour
extreme colors; the _colors.py anchor requires the resolver extraction and
export, while the sibling site requires replacing its direct getattr calls.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 3150-3151: Update the colormap extreme lookup near cmap_under and
cmap_over to support matplotlib’s _rgba_under and _rgba_over attributes, while
retaining compatibility with the existing _under and _over spellings. Mirror the
fallback behavior used by scalar_grid_rgba so contour and colorbar rendering
preserve configured cap colors.
- Around line 3689-3692: Replace the value-based list.index and list.remove
calls in the generated-entry update flow with identity-based lookup and removal,
matching the approach used by Axes._remove_entry. Ensure source_index identifies
the exact source object and each generated entry is removed by object identity
before reinserting generated after the source, avoiding equality comparisons
involving NumPy arrays.
- Around line 738-742: In the piece-filtering loop, replace the data-coordinate
np.allclose(a, b) check with a scale-independent check that appends each piece
when piece_stop is greater than piece_start. Keep the existing interpolation and
result tuple construction unchanged.
In `@tests/pyplot/test_gallery_log_colorbar_blockers.py`:
- Around line 22-26: Update the autouse fixture function _clean to annotate its
yield-based return type as Iterator[None] instead of None, adding the necessary
typing import if the module does not already provide it.
---
Nitpick comments:
In `@js/src/50_chartview.ts`:
- Around line 2539-2571: Remove the duplicate fractionFor closure and reuse
colorbarFraction for all subsequent tick and label positioning calculations,
including the usages near the indicated later lines. Keep the existing
log/linear mapping behavior unchanged.
In `@python/xy/_svg.py`:
- Around line 3953-4093: Update the line rendering loop around line_width so the
numeric width remains available for dash-pattern calculations while its
_num-formatted string is used only for the SVG stroke-width attribute. Compute
the dash values from the original numeric width, avoiding any float() conversion
of the serialized result; preserve the existing minimum width behavior and
output.
In `@python/xy/pyplot/_axes.py`:
- Around line 6286-6299: The colorbar sizing constants in _colorbar_outside_room
are duplicated across renderers and can drift. Define shared exported constants
alongside the existing _svg.layout sizing definitions, then update
_colorbar_outside_room and the raster/JS equivalents to import and reuse them
while preserving the current orientation, placement, padding, and label
calculations.
In `@python/xy/pyplot/_colors.py`:
- Around line 503-511: The local extreme() resolver in Axes.imshow should reuse
the shared resolver from _colors.py so it also handles _rgba_{name} fallbacks
and tuple values consistently. Export the existing helper or its lookup, update
_axes.py’s has_extremes path to call it, and remove the duplicated local
resolution logic while preserving the existing default handling.
In `@python/xy/pyplot/_mplfig.py`:
- Around line 737-747: Extract the shared “select up to ~10 colorbar ticks
closest to zero” algorithm from the line_contour branch and the following
discrete_boundaries branch into a helper such as
_select_colorbar_ticks(locations). Update both branches to call the helper with
their respective source arrays, preserving the existing sampling,
zero-tolerance, and tick-value behavior.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 821-825: Update the 3D RGBA branch in _bilinear_grid to
premultiply RGB channels by alpha before interpolation, interpolate alpha
alongside them, then unpremultiply the resulting RGB using a safe zero-alpha
guard. Preserve the existing per-channel stack shape and behavior for non-RGBA
inputs.
- Around line 478-487: Update the segment-processing block around segments to
quantize all endpoint coordinates vectorized with NumPy rather than unboxing
rows through Python iteration. Preserve the existing tolerance-based key
behavior and skip degenerate segments where both endpoint keys match, while
limiting Python work to constructing endpoints, positions, and adjacency from
the resulting key arrays.
In `@tests/pyplot/test_gallery_log_colorbar_blockers.py`:
- Around line 29-211: The test suite needs coverage for labeling a contour when
another contour already exists on the same axes. Add a regression test near the
existing contourf colorbar test that creates a contourf and then a separate
contour on one axes, calls clabel on the latter, and verifies the labels render
or are associated with the correct contour, exercising the ContourSet.clabel
path and preventing value-based entry lookup failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b088ae39-b99e-427e-91ae-61b6cd53aa2b
📒 Files selected for processing (19)
js/src/00_header.tsjs/src/50_chartview.tspython/xy/_raster.pypython/xy/_svg.pypython/xy/config.pypython/xy/pyplot/__init__.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_colors.pypython/xy/pyplot/_mplfig.pypython/xy/pyplot/_plot_types.pyspec/design/wire-protocol.mdspec/matplotlib/compat-changelog.mdspec/matplotlib/compat.mdtests/pyplot/test_axes_charts.pytests/pyplot/test_contour_label_placement.pytests/pyplot/test_gallery_log_colorbar_blockers.pytests/pyplot/test_p3_option_contracts.pytests/pyplot/test_pdsh_gap_features.py
# Conflicts: # python/xy/_raster.py # python/xy/_svg.py # python/xy/pyplot/_artists.py # python/xy/pyplot/_axes.py
3 participants
Summary
This groups the selected-gallery contour, normalization, and colorbar repairs:
extend, exact listed-color bands, under/over colors, and contouradd_linesoverlays;imshow/pcolormeshcolorbars andpcolormesh(rasterized=True);colorbar(cax=...), including correct placement without shrinking the source axes;Why
The previous contour label path treated marching-square fragments independently, so labels duplicated, collided, ignored tangents, and did not create real inline gaps. Colorbars lost mappable semantics, overwrote one another, sampled explicit listed bands through a named ramp, and treated line contours as filled ramps. Log-normalized meshes and explicit cax placement were also missing.
Visual comparisons
Review-only images from immutable evidence commit
a5d959e; none is part of this PR diff.Automatic contour labels
Manual contour labels
Line-only contour and separate image colorbars
Exact listed colors and extensions
Logarithmic colorbar
Explicit
caxVerification
contour_demo.py,contourf_demo.py,time_series_histogram.py, andsubplots_adjust.pyproduced 12 PNG and 12 SVG outputs;git diff --checkpassed.Remaining approximations
No local browser, native build, or full suite was run under the low-resource verification policy; GitHub CI is the final integrated gate.
Summary by CodeRabbit
caxbehavior and extension color handling.norm="linear"/"log"behavior.