Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BF: ignore CLOSEPOLY after NaN in PathNanRemover #17885

Merged
merged 2 commits into from
Jul 15, 2020

Conversation

brunobeltran
Copy link
Contributor

@brunobeltran brunobeltran commented Jul 11, 2020

PR Summary

Fixes #17868.

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

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 is Flake 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 brunobeltran added topic: path handling Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels Jul 11, 2020
@QuLogic
Copy link
Member

QuLogic commented Jul 11, 2020

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

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

@tacaswell tacaswell added this to the v3.3.0 milestone Jul 11, 2020
@brunobeltran
Copy link
Contributor Author

brunobeltran commented Jul 13, 2020

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 the CLOSEPOLY command is ignored, and
  2. We were already leaving all CLOSEPOLY's in before, including these NaN ones

So this PR only changes the behavior when leaving a CLOSEPOLY in would result in an ill-defined Path.

@brunobeltran
Copy link
Contributor Author

brunobeltran commented Jul 13, 2020

I'm aware that this doesn't play well with PathSimplifier:

>>> 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 full PathSimplifer algorithm yet to know whether or not it's easy to do even better.

@brunobeltran
Copy link
Contributor Author

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

lib/matplotlib/tests/test_path.py Outdated Show resolved Hide resolved
src/path_converters.h Outdated Show resolved Hide resolved
Copy link
Member

@tacaswell tacaswell 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.

@QuLogic QuLogic force-pushed the ignore_closepoly_after_nan branch from b95ce02 to e216e59 Compare July 15, 2020 20:19
Copy link
Member

@QuLogic QuLogic 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.

@QuLogic QuLogic merged commit b7ba9e4 into matplotlib:master Jul 15, 2020
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jul 15, 2020
jklymak added a commit that referenced this pull request Jul 15, 2020
…885-on-v3.3.x

Backport PR #17885 on branch v3.3.x (BF: ignore CLOSEPOLY after NaN in PathNanRemover)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: path handling
Projects
None yet
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