Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

BF: ignore CLOSEPOLY after NaN in PathNanRemover#17885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

brunobeltran
Copy link
Contributor

@brunobeltranbrunobeltran commentedJul 11, 2020
edited
Loading

PR Summary

Fixes#17868.

In short, the currentPathNanRemover always keepsCLOSEPOLY "vertices" around, even if they contain NaN. This is arguably okay (since it is documented in comments that the values ofCLOSEPOLY vertices are ignored). However, because of this, if a CLOSEPOLY occurs before any non-NaN segments of a Path are encountered, thenPathNanRemover (and, by proxy,Path.cleaned(remove_nans=True) will return a malformedPath that starts with aCLOSEPOLY.

Current behavior:

>>>p=Path(    [[np.nan,np.nan], [np.nan,np.nan]],    [1,79])>>>p.cleaned(remove_nans=True)Path(array([[np.nan,np.nan], [0.,0.]]),array([79,0]))

New behavior

>>>p.cleaned(remove_nans=True)Path(array([[0.,0.]]),array([0],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

@brunobeltranbrunobeltran added topic: path handling Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelsJul 11, 2020
@QuLogic
Copy link
Member

I think we ignore the point entirely forCLOSEPOLY, but what happens for something like?

p=Path(    [[0,0], [np.nan,np.nan]],    [1,79])

@tacaswelltacaswell added this to thev3.3.0 milestoneJul 11, 2020
@brunobeltran
Copy link
ContributorAuthor

brunobeltran commentedJul 13, 2020
edited
Loading

what happens for something like?

p=Path(    [[0,0], [np.nan,np.nan]],    [1,79])

Great question. It leaves it as-is. (Same as previous behavior).

>>>p.cleaned(remove_nans=True)Path(array([[0.,0.],       [nan,nan],       [0.,0.]]),array([1,79,0],dtype=uint8))

I think this makes sense because

  1. We document that the vertex attached to theCLOSEPOLY command is ignored, and
  2. We were already leavingallCLOSEPOLY's in before, including theseNaN ones

So this PR only changes the behavior when leaving aCLOSEPOLY in would result in an ill-definedPath.

@brunobeltran
Copy link
ContributorAuthor

brunobeltran commentedJul 13, 2020
edited
Loading

I'm aware that this doesn't play well withPathSimplifier:

>>>p.cleaned(remove_nans=True,simplify=True)Path(array([[0.,0.],       [nan,nan],       [nan,nan],       [0.,0.]]),array([1,2,2,0],dtype=uint8))

but this is also existing behavior, and I haven't taken the time to grok the fullPathSimplifer algorithm yet to know whether or not it's easy to do even better.

@brunobeltran
Copy link
ContributorAuthor

Per the call, added tests that check the "optimized" code path (i.e. emptycodes array) and explicitly added the breaking example from#17868 as a test case.

@brunobeltranbrunobeltranforce-pushed theignore_closepoly_after_nan branch frome90850f tob95ce02CompareJuly 15, 2020 18:12
Copy link
Member

@tacaswelltacaswell left a comment

Choose a reason for hiding this comment

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

Still approve.

@QuLogicQuLogicforce-pushed theignore_closepoly_after_nan branch fromb95ce02 toe216e59CompareJuly 15, 2020 20:19
Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

I pushed a small whitespace fix.

@QuLogicQuLogic merged commitb7ba9e4 intomatplotlib:masterJul 15, 2020
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestJul 15, 2020
jklymak added a commit that referenced this pull requestJul 15, 2020
…885-on-v3.3.xBackport PR#17885 on branch v3.3.x (BF: ignore CLOSEPOLY after NaN in PathNanRemover)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@QuLogicQuLogicQuLogic approved these changes

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.topic: path handling
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

plt.bar with nan input fails rendering in notebook using 3.3.0rc1
3 participants
@brunobeltran@QuLogic@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp