Uh oh!
There was an error while loading. Please reload this page.
refactor(color): ColorSpec - typed color-state boundary - #722
Merged
Conversation
Introduce an explicit color-state type for the IR's color layer: ColorSpec
(colortype in {categorical, continuous, none} + source/color vectors) and
resolve_color(), a pass-through wrapper over _set_color_source_vec that names
the three states the renderers currently infer implicitly from
(source is None, categorical). No caller changes yet; pure addition.
Adds per-return-branch unit tests (none / all-NaN / continuous / categorical).…rs (#700 Step 2a) Replace the implicit two-bool color-state encoding (color_source_vector is None / categorical) with the explicit ColorSpec.colortype across the three element renderers and the shared color/legend helpers. resolve_color() now feeds the 5 resolution sites; _warn_groups(_ignored_continuous), _maybe_apply_transfunc and _add_legend_and_colorbar take colortype directly, which lets the is_continuous_override hack param (added in #720) be dropped. Behaviour-preserving: every predicate is mapped per the three-state table (is None -> continuous, categorical -> categorical, is not None -> != continuous), so the local visual-test failure set is byte-identical before and after.
…dicates (#700 Step 2a) Complete the color-state unification: - delete the misleading `values_are_categorical` intermediate; every site now reads the explicit state directly (`color_spec.is_continuous` etc.), which is the same behaviour expressed honestly (it gated 'not continuous', not 'categorical'). - empower ColorSpec with is_categorical / is_continuous / is_none predicates (on the invariant colortype — safe to read anywhere, unlike the vectors which the renderers mutate after resolution). - shift the points density guard to colortype too. - rename the per-renderer local _spec -> color_spec / outline_color_spec. One genuine fix (behaviour change only in a previously-crashing edge): _warn_groups no longer calls _warn_missing_groups for the 'none' state (a na-array has no .categories) — it now requires colortype == 'categorical'. No baseline shift (the local visual failure set is byte-identical to the prior commit).
…ith_color_vector) (#700) Add the transforms that let the renderers thread one color_spec through the post-resolution vector mutations instead of reassigning source/color in lockstep: - filter(mask): subset both vectors (categorical keeps dtype + drops unused; else coerces to array, matching the groups/transparent-na path). - apply_transfunc: continuous-only color_vector map (folds _maybe_apply_transfunc). - with_color_vector: escape hatch for renderer-specific single-vector rewrites. No caller changes yet; adds unit tests asserting each against the raw equivalent.
…ipeline (#700) Groups-filter and transfunc now run via the immutable transforms (color_spec.filter(keep) / .apply_transfunc(tf)) instead of reassigning source+color in lockstep; the final vectors are unpacked once for the read/draw phase. Byte-identical (visual failure set unchanged).
…ipeline (#700) The rasterize-mask and groups-filter lockstep (src[mask]; col[mask]; remove_unused) — the exact sync footgun the review flagged — now go through color_spec.filter(mask). Byte-identical (visual failure set unchanged).
…_transfunc (#700) Both are now dead — the renderers' groups-filter and transfunc go through ColorSpec.filter / .apply_transfunc. Real subtraction.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #722 +/- ##
==========================================
+ Coverage 77.96% 79.09% +1.12%
==========================================
Files 17 17 Lines 4465 4467 +2 Branches 1003 999 -4 ==========================================
+ Hits 3481 3533 +52 + Misses 633 593 -40 + Partials 351 341 -10
🚀 New features to boost your workflow:
|
…regression) The resolver returned a plain Normalize, silently linearizing LogNorm/PowerNorm/ SymLogNorm for shapes/points/labels (the old _get_collection_shape used the user norm directly). Now copies the norm and fills vmin/vmax, keeping the subclass. Verified: zero baseline shift on the 5 render/colorbar test files (only the untested non-linear-norm path changes); images are unaffected (norm → imshow).
… norm (#700) Thread the outline color through ColorSpec the same way fill already is, so the outline path no longer reassigns two loose vectors in lockstep: - add ColorSpec.align_to_length (colortype-aware) + reuse .filter, dropping the free helpers _align_outline_vector_to_length / _apply_mask_to_outline_vectors - shapes + labels keep one outline_color_spec and unpack once before drawing - categorical pads now carry the na_color hex, not NaN, fixing a crash when a cross-table categorical outline under-annotates the element (to_rgba_array(nan) -> Invalid RGBA argument) - @DataClass(frozen=True, eq=False): array fields make the autogenerated __eq__/__hash__ ambiguous - drop dead with_color_vector Fix the remaining none-colortype crashes the typed state exposes: the groups preamble for shapes + points guarded on `source_vector is not None` (true for the na-array none-state) and called .isin on a plain ndarray; guard on is_categorical to match labels. Restore the degenerate-range norm reset: a constant continuous column (vmin == vmax) now falls back to [0, 1] instead of the colormap floor; LogNorm is exempt (0 is out of its domain). Regression tests: align_to_length pad/truncate, LogNorm subclass preservation, degenerate reset, and render-level no-crash for all-NaN color across labels/shapes/points.
…-not-None (#700) The pad branch keyed on `source_vector is not None`, true for BOTH categorical and the none state — but only a Categorical has `.categories`. A `none`-state outline column (all-NaN) that under-annotates the element crashed with AttributeError. Branch on is_categorical; pad the none state's na-array source with na entries. Adds the missing none-state align test. Also trims the ColorSpec/norm/_warn_groups comments per review (lean, no behaviour change).
…A mapping (#700) The per-row continuous/categorical/object -> RGBA mapping lived twice: inline in _get_collection_shape (fill) and in _color_vector_to_rgba (outline), kept in sync by a "mirrors ..." comment. Hoist it onto ColorSpec.to_rgba(cmap_params) and call it from both: the shapes fill now passes color_spec.to_rgba(...) (so _get_collection_shape only handles the RGBA passthrough + single-color broadcast), and the outline calls outline_color_spec.to_rgba(...). Deletes _color_vector_to_rgba and the now-dead numeric/object cases + their imports. to_rgba is a verbatim port, so outline output is identical; the fill cases used byte-identical expressions. New unit tests lock to_rgba's RGBA (continuous+NaN, categorical, object-mix, RGBA passthrough) independent of visual baselines.
) Replace the loose `colortype` string + `color_source_vector` passing with the spec itself: _warn_groups / _warn_groups_ignored_continuous / _reject_continuous_color_under_density now take a ColorSpec and read is_continuous / is_categorical, retiring the `colortype == "..."` compares. Centralize the 3x groups-filter guard into ColorSpec.groups_keep_mask(groups, na_color) — one place defines when the transparent-na groups filter applies; the renderers keep only their element-specific masking. Fix the datashader-shapes none-state misclassification: `color_by_categorical` keyed on `color_source_vector is not None` (true for the na-array none state) now uses is_categorical. Drops the redundant `categorical` local in labels.
_make_continuous_mappable (the datashader continuous colorbar) expanded a degenerate vmin==vmax to ±0.5, while the pixel path (_resolve_continuous_norm) resets it to [0, 1] — so a constant continuous column drew a colorbar that disagreed with its fill. Use the same [0, 1] fallback in both. Adds a parity test. Intentional pixel change for the datashader-constant-column colorbar.
…700) _add_legend_and_colorbar / _decorate_outline took a loose colortype + fill + outline vector quintet; they now take color_spec + outline_color_spec and decide legend-vs-colorbar-vs-nothing from the predicates. This fixes a latent none-state crash: an all-NaN outline column reached _add_outline_legend, which called .remove_unused_categories() on the na-array source (AttributeError). The none outline now correctly carries no decoration (outline_has_decorations excludes is_none). Categorical/continuous outline behaviour is unchanged. Adds a regression test.
…ate vectors (#700) The renderers unpacked color_spec into loose color_source_vector/color_vector up front, then mutated and passed them around — so our own helpers took the pair instead of the spec. Keep color_spec as the single carrier instead: each post-resolution mutation is a transform (with_color_vector/with_source_vector), make_palette is a ColorSpec method, and _render_centroids_as_points + _add_legend_and_colorbar take the spec. Loose arrays now appear only at the genuine leaves that consume them (ax.scatter/imshow, PatchCollection, _map_color_seg, the datashader funcs): those unpack color_spec.color_vector inline, and the datashader leaves' returns are re-wrapped into the spec. Mechanical, value-for-value identical; set-diff shows zero new failures and the non-visual suite is green.
…r-key (#700) Review caught a regression from the earlier is_categorical switch: shapes datashader keyed color_by_categorical on is_categorical, dropping the none state (all-NaN color column) into the numeric reduction → ValueError: axes don't match array. Restore the old truth set (categorical OR none when a color column is set) via `col_for_color is not None and not is_continuous`; add a datashader regression test. (Points already kept the none state.) Cleanups from the same review: - labels as_points: classify the fresh point-color spec by its own source vector instead of reusing the parent colortype (was constructing invariant-violating none/categorical specs with source=None). - _get_collection_shape: pass the already-(N,4) RGBA fill through instead of a redundant to_rgba_array round-trip (dropped a full-N copy on the hot path). - to_rgba: compute na_rgba only on the numeric branch that uses it. - collapse with_color_vector/with_source_vector into one evolve(**changes).
Commit 017e0b7 changed _make_continuous_mappable's vmin==vmax fallback from ±0.5 to [0,1] to "unify" it with the pixel norm. That was based on a spurious review finding: _make_continuous_mappable is only the datashader colorbar, and _build_ds_colorbar feeds it the user's explicit norm vmin/vmax — so for an explicit vmin==vmax norm the change shifted the colorbar and broke the 4 *_datashader_norm_vmin_eq_vmax_* baselines. matplotlib pixels and a datashader colorbar never co-occur, so there was no real divergence to fix. Restore the original ±0.5 behavior; drop the parity test.
…700) /simplify pass (behaviour-preserving): - continuous-reprocessing: drop the duplicated NaN-count warning across the try/except arms — coerce, then count once via pd.isna and warn once. - has_valid_color: build the distinct-colors set once instead of twice. - _add_legend_and_colorbar: branch on color_spec.is_categorical instead of duck-typing `source is not None and hasattr(remove_unused_categories)`. - to_rgba: use the module-level colors.to_rgba_array (matching _map_color_seg) instead of instantiating ColorConverter(); drop the now-unused import.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
#700 Step 2a: color state becomes one typed, immutable
ColorSpeccarried through the renderers, replacing the implicitsource_vector is None+categorical-bool encoding.What
resolve_color() -> ColorSpecwith explicitcolortype ∈ {categorical, continuous, none}+is_*predicates.filter/apply_transfunc/align_to_length/groups_keep_maskthread one spec through each renderer — the lockstep two-vector footgun is gone for fill and outline.ColorSpec.to_rgba(cmap_params): one continuous/categorical→RGBA mapping for fill and outline (was duplicated in_get_collection_shape+_color_vector_to_rgba)._add_legend_and_colorbar/_decorate_outline) all read the spec's predicates — no morecolortype == "..."orsource is not Noneproxies._filter_groups_transparent_na,_maybe_apply_transfunc,_align_outline_vector_to_length,_apply_mask_to_outline_vectors,_color_vector_to_rgba, theis_continuous_overridehack.eq=False(array fields).Fixes (all in the
nonecolortype, plus norm)none-state crashes:align_to_lengthand_add_outline_legendcalled.categories/.remove_unused_categories()on the na-array; categorical-outlineNaNpad →to_rgba_array(nan); groups.isinon the na-array; datashader-shapes misclassification; a labels rasterize assertion;_warn_missing_groups.LogNorm/PowerNorm, was linearized); restore degeneratevmin==vmax → [0,1](LogNorm exempt) in both the pixel norm and the colorbar mappable so they agree.Verification
to_rgba/norm correctness locked by unit tests (visual can't verify locally).test_plot_*baselines need CI-artifact regen.No public API change;
ColorSpec/resolve_colorare internal.