The checkxy[0] != xy[-1] was meant to check if the user had already included an extraCLOSEPOLY vertex for us in the list of vertices (shape(xy) = (N, 2)).But for single-vertex (degenerate)Polygons, it produced a false positive (sincexy[0] == xy[-1] by definition), leading to malformedPaths that look like:

In [1]:frommatplotlib.patchesimportPolygonIn [2]:pp=Polygon([[0,0]])In [3]:pp.get_path()Out[3]:Path(array([[0.,0.]]),array([79],dtype=uint8))

PR Checklist

Has Pytest style unit tests
Code isFlake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

brunobeltran added topic: path handling Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels

Jul 20, 2020

brunobeltran mentioned this pull request

Jul 20, 2020

Computing the bounding box of a degenerate polygon throws an error#17975

Closed

QuLogic reviewed

Jul 20, 2020

View reviewed changes

lib/matplotlib/patches.py OutdatedShow resolvedHide resolved

brunobeltran force-pushed theclosepoly-clobbers-poly-vertex branch from33cc440 to787bf03Compare

July 20, 2020 23:00

QuLogic approved these changes

Jul 20, 2020

View reviewed changes

Copy link

Member

QuLogic left a comment

Choose a reason for hiding this comment

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

Add a test?

QuLogic added this to thev3.3.1 milestone

Jul 20, 2020

brunobeltran force-pushed theclosepoly-clobbers-poly-vertex branch from787bf03 toe551a27Compare

July 20, 2020 23:13

Copy link

ContributorAuthor

brunobeltran commentedJul 20, 2020

Add a test?

Couldn't decide what to test initially. Settled on testing that for a single point (x, y), the extents of a degenerate polygon with only that point as its vertices has the extentsBbox([[x, y], [x, y]]), which is the behavior where we actually differ from the (buggy) 3.2 behavior.

Copy link

Member

jklymak commentedJul 20, 2020

I'm sure this was covered in the other PR, but what happens if x=np.Nan?

Copy link

ContributorAuthor

brunobeltran commentedJul 21, 2020

tl;dr: NaN works.

ThePolygon class doesn't complain about NaN's or clean them explicitly (currently. You might argue that it should).

SoPolygon([[np.nan, 0]]) creates a path thatwould be correct if there were no NaN's there:Path([[np.nan, 0], [np.nan, 0]], [MOVETO, CLOSEPOLY]). The newPath.get_extents blindly adds all possible extremal values to aBbox.null() (which in this case are all NaN). Adding a NaN to a Bbox is a no-op, (the call chain ends in_path.update_path_extents, which works on aPathNaNRemover<agg:conv_transform<PathIterator>>), so theBbox stays null, correctly indicating there are no non-NaN points in the polygon.

Copy link

ContributorAuthor

brunobeltran commentedJul 21, 2020

CI fail is due to "error allocating resources" on our Mac box?

Copy link

Member

jklymak commentedJul 21, 2020

... the mac CI on azure seems to have issues

dstansby approved these changes

Jul 21, 2020

View reviewed changes

lib/matplotlib/patches.py Outdated

		if self._closed:
		iflen(xy) and (xy[0] != xy[-1]).any():
		ifN == 1 or N > 1 and (xy[0] != xy[-1]).any():

Copy link

Member

dstansbyJul 21, 2020

Choose a reason for hiding this comment

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

Could you add a quick comment about theif statements you've changed just to say what they're doing? And maybenverts is a better variable name than justN?

Copy link

ContributorAuthor

brunobeltranJul 21, 2020•
edited
Loading

Choose a reason for hiding this comment

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

I was reticent to add these comments, because technically the checks here are very brittle as-written, so I didn't want to give people the impression this is something which is okay to do in other code.

The checks are bad because we don't guarantee anything about the coordinates of a CLOSEPOLY vertex, but this code uses the assumption that these vertices will "correctly" match the start point of the curve.

In particular, both Python (e.g.Path.iter_bezier) and C++ code (i.e. handlers foragg:path_flags_close) actively ignore this vertex's value, because it's known to hold "trash" sometimes, especially after cleaning NaN arrays, etc (weird stuff like#17885 and#17914).

But, I added comments that make it more clear what the if statements arenominally doing, since you think it's a good idea. I personally likeN since it matches what all thePolygon docstrings use, but I agree thatnverts is easier to read if you're skimming the code only.

brunobeltran force-pushed theclosepoly-clobbers-poly-vertex branch frome551a27 to1a1c288Compare

July 21, 2020 14:51

BF: for degenerate polygons, add CLOSEPOLY vertex

0d5c018

brunobeltran force-pushed theclosepoly-clobbers-poly-vertex branch from1a1c288 to0d5c018Compare

July 21, 2020 15:02

QuLogic merged commit21c3e3f intomatplotlib:master

Jul 22, 2020

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request

Jul 22, 2020

Backport PRmatplotlib#17982: BF: for degenerate polygons, add CLOSEP…

07847dd

…OLY vertex

meeseeksmachine mentioned this pull request

Jul 22, 2020

Backport PR #17982 on branch v3.3.x (BF: for degenerate polygons, add CLOSEPOLY vertex)#18009

Merged

QuLogic added a commit that referenced this pull request

Jul 22, 2020

Merge pull request#18009from meeseeksmachine/auto-backport-of-pr-17…

5c8392b

…982-on-v3.3.xBackport PR#17982 on branch v3.3.x (BF: for degenerate polygons, add CLOSEPOLY vertex)

Labels

Release critical

For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

topic: path handling

4 participants

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

BF: for degenerate polygons, add CLOSEPOLY vertex#17982

BF: for degenerate polygons, add CLOSEPOLY vertex#17982

Uh oh!

Conversation

brunobeltran commentedJul 20, 2020•
edited
Loading

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

brunobeltran commentedJul 20, 2020

Uh oh!

jklymak commentedJul 20, 2020

Uh oh!

brunobeltran commentedJul 21, 2020

Uh oh!

brunobeltran commentedJul 21, 2020

Uh oh!

jklymak commentedJul 21, 2020

Uh oh!

dstansbyJul 21, 2020

Choose a reason for hiding this comment

Uh oh!

brunobeltranJul 21, 2020•
edited
Loading

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Movatterモバイル変換

Uh oh!

BF: for degenerate polygons, add CLOSEPOLY vertex#17982

BF: for degenerate polygons, add CLOSEPOLY vertex#17982

Uh oh!

Conversation

brunobeltran commentedJul 20, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

brunobeltran commentedJul 20, 2020

Uh oh!

jklymak commentedJul 20, 2020

Uh oh!

brunobeltran commentedJul 21, 2020

Uh oh!

brunobeltran commentedJul 21, 2020

Uh oh!

jklymak commentedJul 21, 2020

Uh oh!

dstansbyJul 21, 2020

Choose a reason for hiding this comment

Uh oh!

brunobeltranJul 21, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brunobeltran commentedJul 20, 2020•
edited
Loading

brunobeltranJul 21, 2020•
edited
Loading