refactor(glyphs)!: move render/prep logic onto the grouped parameter objects - #291
Open
MAfarrag wants to merge 15 commits into
Open
refactor(glyphs)!: move render/prep logic onto the grouped parameter objects#291MAfarrag wants to merge 15 commits into
MAfarrag wants to merge 15 commits into
Conversation
Cohere the grouped render-parameter objects with the logic that consumes their fields, mirroring ColorScaling.build_norm/to_options: the object owns the transform from its fields to render output and the glyph just calls it. - add ColorBar.to_options() and delegate _resolve_colorbar's instance branch to it, so ColorBar is symmetric with the other group objects - add PointOverlay.draw(ax) owning the marker scatter and per-point value labels, replacing the block duplicated across plot() and animate() and the base-glyph Glyph._plot_point_values helper (removed) - add FrameLabel.resolve_location() and FrameLabel.draw(ax, default_size) owning the animate frame-label placement/transform/alignment - relocate the point-value-label tests onto PointOverlay.draw
…he objects Second tier of the same cohesion pass: relocate the remaining single-object transforms off the free function / glyph code onto the group objects that own their fields. - add ColorBar.resolve() owning the None/False/True/instance dispatch and ColorBar.reset_options() owning the colorbar=True reset-to-defaults dict; _resolve_colorbar now just delegates to ColorBar.resolve, so all of the colorbar option-building lives on the object - add PanelLabels.label_for(), PanelLabels.panel_title() and PanelLabels.validate(); facet drops its col_coords/row_coords locals and calls these for per-panel titles/name_dicts and the axis-length checks
Bundle ArrayGlyph's four RGB data-preparation keywords (rgb, surface_reflectance, cutoff, percentile) into a single RgbBands object passed as rgb_bands=, mirroring the render-parameter grouping. The band-selection and stretch transform moves onto the object, and __init__ drops from 10 explicit params to 7 (under Sonar S107). - add RgbBands (indices + surface_reflectance/cutoff/percentile) owning validate(array) and prepare(array); the percentile / reflectance / cutoff stretch lives on the object - ArrayGlyph.__init__ takes rgb_bands instead of the four loose kwargs - prepare_array() and scale_percentile() keep their loose public signatures (and doctests) and now delegate to RgbBands internally; _prepare_sentinel_rgb folds into RgbBands._apply_surface_reflectance (existing cutoff behavior preserved verbatim) - migrate examples and add a migration-guide entry BREAKING CHANGE: ArrayGlyph no longer accepts the loose rgb / surface_reflectance / cutoff / percentile constructor keywords; pass rgb_bands=RgbBands([r, g, b], surface_reflectance=..., cutoff=..., percentile=...) instead. The prepare_array() and scale_percentile() utilities still accept the loose keywords.
Fourth cohesion pass, closing out the single-object logic still living in glyph code: - add DataStyle.for_apply_style(style, hillshade=_UNSET) owning the sentinel-gated apply_style construction the three glyphs each hand-rolled; ArrayGlyph/MeshGlyph/KDEGlyph now call it, and MeshGlyph and KDEGlyph delete their throwaway _UNSET_HILLSHADE sentinels (standardising on DataStyle's own _UNSET) - add ColorBar.specifies_placement() for the location/inside/orientation predicate, removing the last direct ColorBar field reads from glyph code - add Glyph._snapshot_group_options(*groups) for the pre-merge option-snapshot loop duplicated in ArrayGlyph.plot and KDEGlyph.plot (a base-class de-duplication; spans multiple groups so it stays on Glyph, not on any one object)
MAfarrag
changed the title
…index The surface-reflectance cutoff branch indexed `array[0]` (the first row of the band-last array) and clipped the integer band *index* rather than the band's pixel data, so `cutoff` never actually stretched the bands. Clip each band's normalised data to [0, cutoff[band]] and rescale to [0, 1] (a per-band contrast stretch), iterating over the cutoff list. Add a regression test pinning the corrected per-band output.
…methods Cover the group-object methods added on this branch that previously had only indirect (integration/doctest) coverage: - new tests/test_params.py for DataStyle.for_apply_style - TestColorBarMethods (to_options / resolve / reset_options / specifies_placement) in test_colorbar_glyphs - TestSnapshotGroupOptions for Glyph._snapshot_group_options in test_glyph - TestRgbBands, TestFrameLabelMethods, TestPanelLabelsMethods in test_array_glyph - add executable Examples to the docstrings of ColorBar.reset_options and specifies_placement, RgbBands.validate/prepare, PointOverlay.draw, FrameLabel.resolve_location/draw, and PanelLabels.label_for / panel_title / validate
The apply_style refactor to DataStyle.for_apply_style removed the only use of ArrayGlyph's private _Unset/_UNSET, leaving a second sentinel type duplicating the canonical one in styling.params. Delete the dead definition and relocate its repr/identity test onto params._Unset, removing the latent cross-module footgun (passing one module's sentinel into the other's isinstance gate).
…ample and RGB fixture
|
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
Cohere the grouped parameter objects with the logic that consumes their fields. The model is the existing
ColorScaling.build_norm/ every group'sto_options: the object owns the transform from its own fields torender output, and the glyph just calls it. This PR moves the remaining single-object logic out of the glyph
modules onto the objects it belongs to, and — as part of the same pass — groups
ArrayGlyph's four loose RGBband-prep constructor keywords into a new
RgbBandsobject (a breaking change; see below).Render-cohesion moves (non-breaking)
ColorBar—to_options(),resolve()(theNone/False/True/instance dispatch),reset_options()(thecolorbar=Truereset dict), andspecifies_placement()(thelocation/inside/orientationpredicate). Thefree function
_resolve_colorbarnow just delegates; all colorbar option-building lives on the object.PointOverlay.draw(ax)— the marker scatter + per-point value-label drawing, previously duplicated byte-for-byteacross
ArrayGlyph.plotand.animate; the base-glyphGlyph._plot_point_valueshelper is removed.FrameLabel.resolve_location()+draw(ax, default_size)— the animate frame-label placement/transform/alignment.PanelLabels.label_for(),panel_title(),validate()— the facet per-panel title/name_dictbuild and theaxis-length checks (three duplicated raise blocks collapsed).
DataStyle.for_apply_style(style, hillshade=_UNSET)— the sentinel-gatedapply_styleconstruction thatArrayGlyph/MeshGlyph/KDEGlypheach hand-rolled;MeshGlyphandKDEGlyphdrop their throwaway_UNSET_HILLSHADEsentinels.Glyph._snapshot_group_options(*groups)— the pre-merge option-snapshot loop shared byArrayGlyph.plotandKDEGlyph.plot(a base-class de-duplication; it spans multiple group objects so it stays onGlyph).RGB band-prep grouping (BREAKING)
ArrayGlyph's constructor no longer takes the four loose RGB keywords (rgb,surface_reflectance,cutoff,percentile); they are bundled into a newRgbBandsobject passed asrgb_bands=, which owns theband-selection + stretch transform (
validate(array)/prepare(array)). This also drops__init__from 10explicit params to 7. The lower-level
prepare_array()andscale_percentile()utilities keep their loose publicsignatures (and doctests) and now build an
RgbBandsinternally. See the migration-guide entry.No new dependencies.
Issues
This is an internal refactor and does not close an issue.
Type of change
Check relevant points.
Breaking:
ArrayGlyph(array, rgb=[...], surface_reflectance=..., cutoff=..., percentile=...)→ArrayGlyph(array, rgb_bands=RgbBands([...], surface_reflectance=..., cutoff=..., percentile=...)). TheRgbBandsobject is importable fromcleopatra.glyphs.gridded.array_glyph;docs/migration.mddocuments themove. The render-cohesion moves above are behaviour-preserving.
How Has This Been Tested?
Run in the worktree against the external uv environment.
pytest -m "not e2e"→ 2232 passed.--doctest-modulesacross the edited modules (styling/colorbar.py,styling/params.py,glyphs/gridded/array_glyph.py) — incl. the newColorBar.to_options/resolve,DataStyle.for_apply_style,and
RgbBandsexamples, and the migratedArrayGlyph(..., rgb_bands=...)constructor examples.point-value-label tests were relocated onto
PointOverlay.draw, and the 6 RGB constructor test sites migratedto
RgbBands.v0.15.22.Checklist: