Fix themeable._omit inheritance - #1056
Merged
Merged
Conversation
`_omit` was a class attribute consulted by `themeable.properties` to strip keys that mpl.set could not accept on the artist. Because the themeable hierarchy is inverted — general themeables are Python subclasses of specifics — Python's MRO attribute lookup made a general themeable (`rect`, `axis_text`, `line`) silently inherit `_omit` from its first specific base. That filter, calibrated for one specific's artist, was then applied to every artist the general themeable composed via super() chaining. The bug was masked for users by the merge cascade in `Themeables.update`, but visible under direct `themeable.apply` (tests, extensions) and latent for any future reader of `instance.properties` outside `apply_*`. By happenstance, it was also masked by the default values used to create themes: e.g. when defining `theme_gray`, if you change the `rect` to have `fill="pink"` it would not apply!, but with "white" it went unnoticed. Replace `_omit` with `themeable._get_properties(omit=...)` called inside each `apply_*` method. The filter is local to the apply call, tied to the `mpl_artist.set(**)` it feeds, and not subject to MRO attribute leakage. Three previously-defensive `with suppress(KeyError): del props[KEY]` blocks in the title themeables (axis_title_x ha, axis_title_y va, plot_title ha) were also unconditional drops that fold cleanly into the helper call. Composition fix: when the bug was masked, the composition's outer `plot_background` was incidentally transparent. With the fix it correctly carries `rect.fill`, exposing a latent layering issue — at zorder=-1000 the outer covered the per-plot tinted backgrounds at the same zorder. Move the composition outer to zorder=-2000 so per-plot backgrounds layer above it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1056 +/- ##
==========================================
- Coverage 86.84% 86.81% -0.03%
==========================================
Files 203 203
Lines 13788 13759 -29
Branches 1689 1689
==========================================
- Hits 11974 11945 -29
Misses 1265 1265
Partials 549 549 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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.
_omitwas a class attribute consulted bythemeable.propertiesto strip keys that mpl.set could not accept on the artist. Because the themeable hierarchy is inverted — general themeables are Python subclasses of specifics — Python's MRO attribute lookup made a general themeable (rect,axis_text,line) silently inherit_omitfrom its first specific base. That filter, calibrated for one specific's artist, was then applied to every artist the general themeable composed via super() chaining.The bug was masked for users by the merge cascade in
Themeables.update, but visible under directthemeable.apply(tests, extensions) and latent for any future reader ofinstance.propertiesoutsideapply_*. By happenstance, it was also masked by the default values used to create themes: e.g. when definingtheme_gray, if you change therectto havefill="pink"it would not apply!, but with "white" it went unnoticed.Replace
_omitwiththemeable._get_properties(omit=...)called inside eachapply_*method. The filter is local to the apply call, tied to thempl_artist.set(**)it feeds, and not subject to MRO attribute leakage. Three previously-defensivewith suppress(KeyError): del props[KEY]blocks in the title themeables (axis_title_x ha, axis_title_y va, plot_title ha) were also unconditional drops that fold cleanly into the helper call.Composition fix: when the bug was masked, the composition's outer
plot_backgroundwas incidentally transparent. With the fix it correctly carriesrect.fill, exposing a latent layering issue — at zorder=-1000 the outer covered the per-plot tinted backgrounds at the same zorder. Move the composition outer to zorder=-2000 so per-plot backgrounds layer above it.