Fix Line3D.draw requiring .shape on array-like coordinates - #32128
Fix Line3D.draw requiring .shape on array-like coordinates#32128thc1006 wants to merge 3 commits into
Conversation
set_data_3d documents its parameters as array-like and checks only that each is iterable, storing them as given. draw then read self._verts3d[0].shape, so a list raised AttributeError. The access sits inside the invalid-scale masking branch, so a finite list draws and only a non-finite coordinate reaches it. That is why this survived until 3.11 added the branch. Use np.shape, which leaves what get_data_3d returns unchanged. Closes matplotlib#32127
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
The test covered one way in: a list of all-NaN coordinates on a linear scale. The masking branch is reached three ways, and the earlier commit message said only a non-finite coordinate could get there, which is wrong. It is entered when a coordinate is non-finite, when it is outside the active scale's domain, or when it is outside the view limits and axlim_clip is enabled. The last two need no non-finite value at all: a plain 0.0 on a log axis reaches it, and so does a finite in-domain point that is simply off screen. Parametrized over list and tuple, and over five ways in: NaN, inf, a finite value outside a log domain, a finite value masked by axlim_clip, and a multi-vertex line where only one vertex is masked. Against a released 3.11.1 with only art3d.py replaced, all ten fail before this change and pass after, and the rest of test_art3d.py stays green.
|
The Azure Windows py313 leg is red on That is the Tk backend under a 60 second subprocess deadline, not anything this PR touches. The diff is Leaving it rather than pushing an empty commit, since a re-run is yours to trigger and I would rather not churn the branch. |
|
This reads to me as completely AI-generated, and it's not clear if this or coercing |
|
⏰ This pull request might be automatically closed in two weeks from now. Thank you for your contribution to Matplotlib and for the effort you have put into this PR. This pull request does not yet meet the quality and clarity standards needed for an effective review. Project maintainers have limited time for code reviews, and our goal is to prioritize well-prepared contributions to keep Matplotlib maintainable. Matplotlib maintainers cannot provide one-to-one guidance on this PR. However, if you ask focused, well-researched questions, a community member may be willing to help. 💬 To increase the chance of a productive review:
As the author, you are responsible for driving this PR, which entails doing necessary background research as well as presenting its context and your thought process. If you are a new contributor, or do not know how to fulfill these requirements, we recommend that you familiarize yourself with Matplotlib's development conventions or engage with the community via our Discourse or one of our meetings before submitting code. If you substantially improve this PR within two weeks, leave a comment and a team member may remove the |
Dear @scottshambaugh , |
PR summary
Closes #32127.
Line3D.set_data_3ddocuments its parameters asarray-like, checks only that each argument is iterable, and stores them as given:drawthen readself._verts3d[0].shape, so a list raisedAttributeError: 'list' object has no attribute 'shape'.The access lives inside the invalid-scale masking branch, so a finite list draws and only an invalid coordinate reaches it. That is why it went unnoticed.
The requirement arrived in two steps. d18c52b introduced
_verts3d[0].shapebehindif self._axlim_clip:, so it applied only to lines that had opted into clipping. 629d396 then added_scale_invalid_mask, which runs unconditionally, and changed the guard toif np.any(scale_mask):. Nothing aboutset_data_3dchanged; the block consuming what it stores did. 3.10.9 has neither and draws all of these.NaN is not the only trigger. On 3.11.1 with lists in
_verts3d,AttributeErroralso comes frominf, from a value<= 0on a log scale, and from an out-of-view point withaxlim_clip=True. Tuples fail the same way. The log-scale row is the awkward one:_scale_invalid_maskwas added so that data outside a scale's valid domain is ignored rather than breaking the plot, and here it raises instead.np.shapeaccepts both, and unlike converting in the setter it leaves whatget_data_3d()returns unchanged, so a caller that reads back what it passed still gets it.PR checklist
pytestpassesdoc/users/next_whats_new/if major new feature — n/adoc/api/next_api_changes/if API changed — no API change; this makes the documented contract holdVerification
Against a released 3.11.1 with only
art3d.pyreplaced, so the comparison is like for like:test_art3d.pyNote on a sibling occurrence
Path3DCollection.do_3d_projectionhas the same pattern one screen down:I left it alone.
_offsets3dis private and its public writer routes throughjuggle_axes(xs, ys, np.atleast_1d(zs), zdir)withxs, yscoming fromget_offsets(), so it holds arrays;ax.scatterwith a NaN draws fine. Assigning_offsets3da tuple of lists directly does reproduce the same error, so it is the same defect, just not one reachable through public API. Happy to include it here if you would rather the two were consistent.I checked the other users of
_scale_invalid_maskand_viewlim_maskthe same way, driving each through its public entry point with list input and a triggering coordinate:Line3DCollection,Poly3DCollection,ax.scatter,ax.text,plot_surfaceandax.plotall draw, because each converts to arrays on the way in. The twoLine3Dsetters are the exception, and they are the documented way to update an existing 3D line in place. Details in #32127.How this was found
Downstream code updating a rocket marker each frame, written against the advice in #22308 to use
set_data([a], [b])andset_3d_properties([c]). That produces exactly this state, and it drew on 3.10 and raised on 3.11 the moment a coordinate went non-finite, which happens before the object being plotted has a position.Update: the test was covering one way in, of three
set_data_3dwith a list of NaN on a linear scale was the only case. The masking branch is entered when a coordinate is non-finite, when it is outside the active scale's domain, or when it is outside the view limits withaxlim_clipenabled. The last two need no non-finite value at all:0.0on a log axis reaches it, and so does a finite in-domain point that is simply off screen. My first commit message said only a non-finite coordinate could get there, which was wrong.Parametrized over
listandtuple, and over five ways in: NaN, inf, a finite value outside a log domain, a finite value masked byaxlim_clip, and a multi-vertex line where only one vertex is masked.art3d.pyonlytest_art3d.pyBackport
3.11.1 is affected, so this wants a
v3.11.xbackport once it lands. Flagging rather than labelling, since I cannot set milestones.Two findings from the same audit, filed separately
Neither belongs in this PR, and neither blocks it.
#32129 is the one I would look at next.
ScaleBase.val_in_rangecallslimit_range_for_scalewith an array, and when a third-party scale is written for scalars, which is the shapeLogScale.limit_range_for_scaleitself uses, theexcept (TypeError, ValueError)fallback returns "nothing is in range"._scale_invalid_masknegates that, so every point on the axis is masked and every 3D artist silently disappears with ordinary finite data. Measured: line 0/4 and scatter 0/4 finite points after projection, no warning, no exception. Silent deletion seems worse than the crash this PR fixes.Line3D.set_data_3dalso accepts two or four coordinate sequences, becausezip('xyz', args)truncates silently. The failure then surfaces at draw time asTypeError: _scale_invalid_mask() missing 1 required positional argument, which names an internal function at the user. Pre-existing and unrelated to the masking change; I have not filed it separately, and can if it is wanted.