Skip to content

FIX: _safe_first_finite on all non-finite array #25547

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 12, 2023
Merged

FIX: _safe_first_finite on all non-finite array #25547

merged 1 commit into from
May 12, 2023

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Mar 25, 2023 •
edited
Loading

PR Summary

This function used to be named _safe_first_non_none (#23751), and at v3.6.0 we have

import numpy as np
import matplotlib.cbook

arr = np.full(5, np.nan)
print(matplotlib.cbook._safe_first_non_none(arr))
nan

So this PR reinstates previous behaviour. Currently on main, _safe_first_finite raises StopIteration when passed an all-nan array.

Fixes #18294 and fixes #24818. The examples from both those issues now successfully produce empty plots. The example from #18294 no longer throws the originally reported warning, but the example from #24818 does throw

/home/ruth/git_stuff/matplotlib/lib/matplotlib/axes/_axes.py:1185: RuntimeWarning: All-NaN axis encountered
  miny = np.nanmin(masked_verts[..., 1])
/home/ruth/git_stuff/matplotlib/lib/matplotlib/axes/_axes.py:1186: RuntimeWarning: All-NaN axis encountered
  maxy = np.nanmax(masked_verts[..., 1])

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@rcomer rcomer added the PR: bugfix Pull requests that fix identified bugs label Mar 25, 2023
@rcomer rcomer mentioned this pull request Mar 25, 2023
Closed
2 tasks
@rcomer rcomer added this to the v3.7.2 milestone Mar 25, 2023
@rcomer
Copy link
Member Author

rcomer commented Mar 26, 2023

I see that a different approach was taken to handle the change in behaviour for bar #24149.

@rcomer rcomer mentioned this pull request Mar 26, 2023
Closed
@oscargus
Copy link
Member

oscargus commented Apr 7, 2023

I think that this approach is quite a bit simpler than the one used in #24149. On the other hand it is a bit more explicit what happens there.

My suggestions:

  • Pick the first element of the array instead of np.nan (may be all np.inf and then returning np.nan is not so obvious...)
  • Update the doc-string and state what happens if no parameter is finite
  • Update the doc-string to replace skip_none with skip_nonfinite (old issue)
  • (Change bar to use the new method?)

@rcomer
Copy link
Member Author

rcomer commented Apr 7, 2023

Thanks @oscargus your suggestions make sense to me. I’ll put this in draft until I have time to implement them.

@rcomer rcomer marked this pull request as draft April 7, 2023 18:43
@rcomer rcomer force-pushed the safe_first_finite branch from 237e492 to 82a7eac Compare April 8, 2023 12:46
@rcomer rcomer marked this pull request as ready for review April 8, 2023 12:47
@rcomer rcomer force-pushed the safe_first_finite branch from 82a7eac to f8bc604 Compare April 8, 2023 12:53
@rcomer rcomer changed the title FIX: _safe_first_finite on all nan array FIX: _safe_first_finite on all non-finite array Apr 8, 2023
jklymak
jklymak reviewed Apr 18, 2023
View reviewed changes
lib/matplotlib/axes/_axes.py
except StopIteration:
# this means we found no finite element, fall back to first
# element unconditionally
x0 = cbook.safe_first_element(x0)

try:
x = cbook._safe_first_finite(xconv)
except (TypeError, IndexError, KeyError):
Copy link
Member

Choose a reason for hiding this comment

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

I think there a few other places where StopIteration is try/excepted for _safe_first_element? Did you check all those?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't, but I have now. I only found these two:

matplotlib/lib/matplotlib/dates.py

Lines 1828 to 1831 in 7103779

try:
x = cbook._safe_first_finite(x)
except (TypeError, StopIteration):
pass

matplotlib/lib/matplotlib/units.py

Lines 182 to 185 in 7103779

try: # If cache lookup fails, look up based on first element...
first = cbook._safe_first_finite(x)
except (TypeError, StopIteration):
pass

These both predate the change that led to StopIteration being thrown for all-nan arrays. The commit that introduced the first says it was for zero length objects, and these will still trigger a StopIteration when the logic arrives here. I'm not sure about the second - should units.Registry.get_converter also be able to handle zero length objects?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really know - just noted that it happens a few other places, and the more logic we can move into a consistent helper function, the better in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

the more logic we can move into a consistent helper function, the better in my opinion.

I certainly agree with that. I wonder if further consolidation should wait for a separate PR though, given recent clarifications about what should and should not be backported

  • my current change fixes a regression from v3.6, so it would be good to get in a patch release
  • a change that tidies the code but (hopefully) makes no difference to the user should maybe wait till v3.8

@ksunden ksunden merged commit b61bb0b into matplotlib:main May 12, 2023
@meeseeksmachine meeseeksmachine mentioned this pull request May 12, 2023
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request May 12, 2023
@ksunden @meeseeksmachine
Backport PR matplotlib#25547: FIX: _safe_first_finite on all non-fi…
98b3fa6 
…nite array
greglucas added a commit that referenced this pull request May 13, 2023
@greglucas
Merge pull request #25879 from meeseeksmachine/auto-backport-of-pr-25…
fbde951 
…547-on-v3.7.x

Backport PR #25547 on branch v3.7.x (FIX: `_safe_first_finite` on all non-finite array)
@rcomer rcomer deleted the safe_first_finite branch May 30, 2023 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak left review comments

@ksunden ksunden ksunden approved these changes

@oscargus oscargus oscargus approved these changes

Assignees
No one assigned
Labels
PR: bugfix Pull requests that fix identified bugs
Projects
None yet
Milestone
v3.7.2
4 participants
@rcomer @oscargus @jklymak @ksunden