fix(analysis): compose vector fields onto a caller ax instead of clearing it - #1175
Merged
Merged
Conversation
…ring it plot_vector_field(ax=host) documents composition onto a shared map, but cleopatra cleared the host axes on render, so a scalar plot() drawn first was wiped — the scalar+vector map the ax parameter exists for was impossible (#1128). cleopatra >=0.39.0 (already the viz floor) added an opt-in plot(compose=...) that keeps the host's artists. Forward compose=True whenever the caller supplies ax; when we create our own axes there is nothing to preserve, so it stays off. - before: scalar.plot(ax=host) -> 1 image; plot_vector_field(ax=host) -> 0 images (scalar destroyed) - after : the scalar image survives (1) and the quiver is drawn on top Also documents the already-working thin= passthrough (draw every nth grid point so a dense quiver/barbs grid is not one arrow per cell) and adds regression tests for the composition (len(ax.images) survives) and the thinning. The issue's third ask — a solid color= for the arrows — is a cleopatra capability gap, not a pyramids wiring issue: cleopatra 0.39.0 always colours arrows by magnitude and plot(color=) takes a ColorScaling object, raising TypeError on a plain string. That belongs upstream. Refs #1128
4 tasks
/test sweep of plot_vector_field: line coverage was complete but the `if y[0] > y[-1]` flip had an uncovered false branch — every existing vector-field test uses a north-up (descending-y) raster, so the y-not-flipped path (a south-up, ascending-y grid) was never exercised. Add a south-up case; plot_vector_field is now 100% line + branch covered (no missing lines/branches in the method span). 11 vector-field tests pass.
The extended description claimed a north-up geotransform is "assumed", but the method flips a descending x/y to ascending and leaves an already-ascending (south-up) axis as-is, so orientation is handled — the real assumption is only that the grid is axis-aligned (unrotated; the rotation terms are ignored). Reword to state that accurately. No code change; analysis.py doctests: 23 passed.
…-adds plot_vector_field docstring gains three clarifications (no code change): - Solid-colour arrows: cleopatra colours arrows by magnitude through cmap; a one-colour colormap gives a single solid colour, e.g. cmap=matplotlib.colors.ListedColormap(["black"]). Verified end-to-end: plot_vector_field(cmap=ListedColormap(["black"])) -> unique arrow colours=1, rgba [0,0,0,1]. (This is #1128's P3 — achievable today, no cleopatra change.) - thin applies to quiver/barbs only; streamplot ignores it (with a warning) and uses density. - A caller-supplied ax is preserved, so calling plot_vector_field again on the same ax adds another field on top rather than replacing it (verified: repeated calls leave 2 collections); start from a fresh axes to redraw. analysis.py doctests: 23 passed.
…iew round 1) Resolves the round-1 review findings on the #1128 tests: - M1: lock the additive semantics — a second plot_vector_field(ax=host) composes another quiver on top rather than replacing it (verified collections 1 -> 2), so the documented behaviour is a tested decision. - L1: assert composing with default add_colorbar adds no extra colorbar axes and preserves the host image (fig.axes 1 -> 1), locking cleopatra's compose-colorbar contract from pyramids' side. - L2: the ascending-y (no y-flip) test now asserts the quiver offsets equal the un-flipped meshgrid(x, y), not just non-None, so a mirror/placement regression is actually caught. - N1: the compose test asserts the added collection is a Quiver, not merely that some collection exists. 13 vector-field tests pass.
Round-2 review (M1): the ascending-y test asserted sorted quiver offsets equal the sorted meshgrid — but for a full grid the point set is invariant to a y reversal (and column-wise sorting decouples x from y), so it could not detect the row-mirror it claimed to guard, and #1128 is a placement fix. Rewrite with u = row index (asymmetric across rows) and assert each arrow's (u, v) via quiver.U / quiver.V matches the un-flipped data cell-for-cell, plus the per-cell positions. Verified this distinguishes a mirror: q.U == u.ravel() while q.U != flipud(u).ravel(). 13 vector_field tests pass.
Round-2 review (M2): the docstring documented the solid-colour cmap recipe but did not warn that a bare color= silently no-ops. Verified plot_vector_field( color="black") still renders magnitude colours with no error. Add a line: a bare color= is filtered out by VectorGlyph.filter_kwargs (cleopatra's color is a magnitude scale object, not a solid colour), so use cmap for a solid colour. analysis.py doctests: 23 passed.
Closes #1128's Problem 3 directly: `plot_vector_field(color="black")` now renders solid single-colour arrows. cleopatra colours arrows by magnitude and has no scalar quiver colour (its `color=` is a magnitude `ColorScaling`), so pyramids translates a matplotlib colour into a one-colour colormap (`ListedColormap([color])`) before handing it to VectorGlyph; `color=` and `cmap=` are mutually exclusive (raises ValueError). Previously `color="black"` was silently dropped by `filter_kwargs` (arrows stayed magnitude-coloured, no error). before/after (verified): plot_vector_field(color="black") arrow colours: before -> many (magnitude); after -> 1 unique rgba [0,0,0,1] Two tests added (both fail without the change): solid-colour arrows, and the color=/cmap= conflict. 15 vector_field tests pass; analysis.py doctests 23 pass; ruff + mypy clean.
… S5778) The color=/cmap= conflict test built ListedColormap(["red"]) inside the pytest.raises block, so two calls could throw (SonarCloud python:S5778). Construct the colormap before the block, leaving only plot_vector_field as the raising call. Test still passes.
… example /docstring pass on the Option B color= feature: add the color=/cmap= mutual exclusion to plot_vector_field's Raises section and a solid-black-arrows example. analysis.py doctests: 23 passed.
…iew M1)
Round-1 review (color= feature) M1: an invalid color= was not validated —
ListedColormap([color]) constructs cleanly, so a bad colour slipped past the
call and (verified) did NOT raise at all through the Agg render; it would only
fail deep in matplotlib when the figure is rasterised, naming neither color=
nor the method. Reject it eagerly with is_color_like, mirroring the color=/cmap=
conflict guard.
before/after:
plot_vector_field(color="notacolour")
before -> DID NOT RAISE (silently accepted)
after -> ValueError: color= must be a matplotlib colour, got 'notacolour'
Failing-first test added (test_plot_vector_field_invalid_color_raises); 16
vector_field tests pass; ruff + mypy clean.
…view L1)
Round-1 review (color= feature) L1: a standalone solid color= still drew a
magnitude colorbar (real ticks, one flat colour) — a scale that no longer
exists. Default add_colorbar=False when color= is set; an explicit
add_colorbar=True still wins (setdefault).
before/after:
plot_vector_field(color="black") # standalone
before -> fig.axes == 2 (misleading magnitude colorbar)
after -> fig.axes == 1 (no colorbar); add_colorbar=True still -> 2
Failing-first test added (test_plot_vector_field_solid_color_suppresses_colorbar);
17 vector_field tests pass; ruff + mypy clean.
…lib import Round-1 review (color= feature) nits: - N1: reword the color= docstring/comment from "every arrow" to "the whole field (arrows, barbs, or streamlines)" — the one-colour cmap applies to all three kinds, not just quiver. Also note the colorbar is suppressed by default for a solid color= (the L1 behaviour). - N2: add a one-line comment on the inline `from matplotlib.colors import ...` explaining it is a [viz]-extra-only deferral (matplotlib ships with cleopatra and is TYPE_CHECKING-only at module top, so a top-level import would break a bare install) — consistent with the adjacent require_cleopatra()+VectorGlyph inline import. analysis.py doctests: 23 passed; 17 vector_field tests pass.
…re reads
Round-2 review (color= feature):
- L1: the color=/cmap= conflict guard keyed on presence (`if "cmap" in kwargs`)
while color=None means unset, so a caller forwarding `cmap=None` (its default)
with color= was wrongly rejected. Gate on value: `if kwargs.get("cmap") is not
None`. before/after:
plot_vector_field(color="black", cmap=None)
before -> ValueError: pass either color= or cmap=, not both
after -> solid black arrows (one colour)
- L3: move the cheap, data-independent color/cmap validation above the two
read_array(band=...) calls so a bad color= fails fast without loading the
bands (it needs neither the arrays nor the axis flip).
Failing-first test added (test_plot_vector_field_color_with_cmap_none_is_allowed);
18 vector_field tests pass; ruff + mypy clean.
…orScaling color=
Round-2 review (color= feature):
- L2: add the is_color_like rejection ("color= must be a valid matplotlib
colour") to plot_vector_field's Raises section — it was a whole validation
path missing from the contract.
- L4: note that, unlike Dataset.plot's color= (a magnitude ColorScaling),
plot_vector_field's color= is a solid matplotlib colour, so a caller who
copies color=ColorScaling(...) across isn't silently confused.
analysis.py doctests: 23 passed.
Round-2 review noted color= was only tested with quiver though the docstring promises the whole field. Add a parametrized test asserting the returned mappable renders one unique colour (black) for each kind — verified im.cmap(im.norm(im.get_array())) yields a single [0,0,0,1] for quiver (Quiver), barbs (Barbs), and streamplot (LineCollection). 21 vector_field tests pass.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
plot_vector_field(ax=host)documents composing arrows onto a shared map, but the render cleared the host axes(via cleopatra's live-axes reset), so a scalar
plot()drawn first was wiped — the scalar+vector map theaxparameter exists for was impossible.
cleopatra
>=0.39.0(already thevizfloor) added an opt-inplot(compose=...)that keeps the host's artists.plot_vector_fieldnow forwardscompose=Truewhenever the caller suppliesax. It also gains a solidcolor=(translated to a one-colour colormap, since cleopatra has no scalar quiver colour) and documents the
thin=passthrough.
No new dependencies (cleopatra floor unchanged at
>=0.39.0).Issues
compose=Truewhen anaxis supplied, so the host layer is preserved.thin=(cleopatra 0.39.0), documented and regression-tested.color=reaches the arrows):plot_vector_field(color="black")now renders solid single-colour arrows(pyramids translates a matplotlib colour into a one-colour colormap);
color=andcmap=are mutually exclusive.Type of change
How Has This Been Tested?
Headless (cleopatra Agg backend), pinned dev env —
tests/dataset/plot/test_plot_dataset.py:test_plot_vector_field_ax_preserves_host_layer— scalar+vector compose keepshost.images == 1, adds aQuiver.test_plot_vector_field_ascending_y_keeps_vectors_in_place— south-up path keeps each(u, v)on its cell.test_plot_vector_field_repeated_ax_calls_add_fields— composing again on the sameaxadds a field.test_plot_vector_field_compose_default_colorbar_adds_no_extra_axes— composing adds no colorbar axes.test_plot_vector_field_thin_reduces_arrow_count—thin=3draws strictly fewer arrows.test_plot_vector_field_color_makes_solid_arrows+..._color_and_cmap_conflict_raises— P3.analysis.pydoctests pass; ruff (0.15.22) + mypy clean.Checklist: