fix(array_glyph): draw into a figure supplied without an axes - #331
Merged
Conversation
ArrayGlyph(arr, fig=fig).plot() -- a figure bound without an axes -- left
self.ax None and then crashed in _clear_projection_frame with an internal
AttributeError ('NoneType' object has no attribute '_cleo_projection_frame'),
while ax-alone and fig+ax both worked.
- Derive the axes from the figure at render time (its current axes, or a fresh
one) in both plot() and animate(), so fig-alone draws into the caller's
figure. Deriving at render rather than construction means a later
plot(ax=...) override leaves no stray axes on the bound figure.
- Guard _clear_projection_frame(None) to a no-op (return False) as
defense-in-depth, so a not-yet-resolved axes can never raise there.
- Add regression tests for all four fig/ax forms plus the None guard.
Closes #326
MeshGlyph shared the same fig-without-ax root cause as ArrayGlyph: a figure bound at construction with no axes left self.ax = None, so the first plot() or animate() render crashed on a None axes. Mirror the ArrayGlyph resolution: when a figure is bound but its axes is unresolved, draw into the figure's first axes (or a fresh add_subplot(111) if it has none) in both plot() and animate().
plot()'s fig-only branch already clears _auto_figure/_owns_figure so teardown never tightens or repaints a caller-owned figure; animate()'s mirror branch set only self.ax. Set both flags there too so the two branches are identical and the caller's figure is never treated as cleopatra-owned.
The fig-only path adopts self.fig.axes[0] (the figure's first axes), but the fig docstring and code comment called it the 'current axes' — which diverges from fig.axes[0] for multi-axes figures. Reword both to 'first axes' to match the implementation, and add the fig-derived tier to plot()'s ax resolution-priority list so the two docstrings agree.
TestFigAxResolution exercises MeshGlyph's four construction forms (neither, ax only, fig only, fig+ax) plus both animate() fig-only halves, asserting the mesh is actually drawn (glyph.im) and the right axes is adopted -- pinning the new fig-only resolution branches in plot() and animate().
…r/ownership Close the coverage gaps the review flagged on TestFigAxResolution: add test_animate_fig_only_empty_figure_creates_axes for animate()'s untested add_subplot(111) half; assert glyph.im is not None (a render actually happened, not just axes resolution) on the fig-only draw tests; assert _owns_figure / _auto_figure stay False after a fig-only render; and assert the plot(ax=) override switches self.fig to the override's figure.
The animate() fig-only resolution branch had its True edge covered but not the False edge (fig AND ax bound at construction, so the axes is used without re-resolution). Add test_animate_fig_and_ax_uses_bound_axes to ArrayGlyph and MeshGlyph, closing the partial branch (array_glyph 4485->4491, mesh_glyph 1338->1341).
Lock the contracts the animate fig-only branch actually provides: assert the ArrayGlyph animate() test resets _owns_figure/_auto_figure to False (the L1 lines were otherwise unexercised), and assert glyph.im is not None on the three MeshGlyph animate tests so they confirm a frame was drawn, not just that the axes resolved.
Mirror ArrayGlyph.plot()'s ax docstring: spell out the full resolution priority (bound axes > an axes derived from a figure bound at construction > a new figure/axes) instead of the terse 'uses stored axes or creates new', which did not mention the fig-only tier this PR added to MeshGlyph.plot().
SonarCloud S9073 (MAJOR) flagged six 'assert A and B' composite assertions in the new TestFigAxResolution classes. Split each into separate assert statements so a failure pinpoints which condition broke.
|
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
Fixes a crash when an
ArrayGlyphis bound to a figure without an axes.ArrayGlyph(arr, fig=fig).plot()left
self.axasNoneand then raised an internalAttributeErrorin_clear_projection_frame(
'NoneType' object has no attribute '_cleo_projection_frame'), whileax-alone andfig+axboth worked —an asymmetry surfaced while documenting pyramids'
Dataset.plot(fig=…, ax=…)passthrough (serapeum-org/pyramids#1077).add_subplotif it has none —in both
plot()andanimate(), so a figure supplied without an axes is drawn into rather than leftNone.Doing it at render (not construction) means a later
plot(ax=…)override still wins and leaves no stray axeson the bound figure.
_clear_projection_frame(None)to a no-op (returnsFalse) as defense-in-depth, so a not-yet-resolvedaxes can never raise there.
ax-alone andfig+axbehaviour is unchanged.No new runtime dependencies.
Issues
Type of change
Check relevant points.
How Has This Been Tested?
New
TestFigAxResolutionclass intests/test_array_glyph.pycovering the issue's Definition of Done:neither(own figure),axonly (adopts axes),figonly (draws into thesupplied figure — the regression), and
fig+ax(uses the axes).figonly reuses the figure's existing axes; on an empty figure it adds one.plot(ax=…)override on a fig-bound glyph wins and leaves no stray axes on the bound figure.animate(fig=…)(the same latent gap) also draws into the supplied figure._clear_projection_frame(None)is a no-op (False).Reproduce (external uv env, worktree on
src):tests/test_array_glyph.py::TestFigAxResolution(8 passed)pytest tests/ -q→ 2684 passed;ruff checkcleanChecklist: