Skip to content

fix: don't mutate the source stops in ColorStops.reversed() - #147

Merged
tlambert03 merged 3 commits into
pyapp-kit:mainfrom
matthiasschabel:fix/colorstops-reversed-mutation
Aug 10, 2026
Merged

fix: don't mutate the source stops in ColorStops.reversed()#147
tlambert03 merged 3 commits into
pyapp-kit:mainfrom
matthiasschabel:fix/colorstops-reversed-mutation

Conversation

@matthiasschabel

Copy link
Copy Markdown
Contributor

ColorStops.reversed() corrupts the object it is called on (src/cmap/_colormap.py:1164).

self._stops[::-1] is a view, so rev_stops[:, 0] = 1 - rev_stops[:, 0] writes through to the source: every position p becomes 1 - p while the colors stay where they were. A red/green/blue ramp at [0, 0.5, 1] is left holding [1, 0.5, 0] — descending stops the constructor itself rejects with "Color stops must be in ascending position". __init__ stores arrays uncopied (:918), so the returned object aliases that buffer too.

cmap = Colormap(["red", "green", "blue"])
cmap.reversed()
cmap.lut(5)[:, :3]  # -> red, red, blue, blue, blue  (green is gone)

Two .reversed() calls on the same object also disagree, since the first corrupts the input to the second.

Copying the reversed view before rewriting positions fixes both halves, and matches shifted() twelve lines below, which already copies before rewriting positions.

Nothing exercised this: the existing reversal assertions use the reversed() builtin (__reversed__), a different method, and cmap's two internal callers discard the corrupted source immediately.

🤖 Generated with Claude Code

`self._stops[::-1]` is a view, so rewriting the position column wrote
through to the object being reversed: its positions became `1 - p` while
its colors stayed put, leaving stops in descending order that the
constructor itself rejects. Evaluating the original colormap afterwards
gave a wrong ramp, and a second `.reversed()` call disagreed with the
first. `__init__` stores arrays uncopied, so the returned object aliased
the same buffer.

Copying the reversed view first fixes both, and matches `shifted()`,
which already copies before rewriting positions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewed-By: Codex (gpt-5.6-sol, reasoning effort xhigh)
@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.70%. Comparing base (2c4b86e) to head (c21861e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #147   +/-   ##
=======================================
  Coverage   95.70%   95.70%           
=======================================
  Files         168      168           
  Lines        2188     2188           
=======================================
  Hits         2094     2094           
  Misses         94       94           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@matthiasschabel
matthiasschabel marked this pull request as ready for review August 9, 2026 21:39

@tlambert03 tlambert03 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch thanks

@tlambert03
tlambert03 enabled auto-merge (squash) August 10, 2026 07:39
@tlambert03
tlambert03 merged commit f94408c into pyapp-kit:main Aug 10, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants