FIX: `_safe_first_finite` on all non-finite array by rcomer · Pull Request #25547 · matplotlib/matplotlib · GitHub

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

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

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?)

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

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

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?

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.

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

…nite array

…547-on-v3.7.x Backport PR #25547 on branch v3.7.x (FIX: `_safe_first_finite` on all non-finite array)

rcomer added the PR: bugfix Pull requests that fix identified bugs label Mar 25, 2023

rcomer mentioned this pull request Mar 25, 2023

Fix errorbar crash on nan values #24823

Closed

2 tasks

rcomer added this to the v3.7.2 milestone Mar 25, 2023

rcomer mentioned this pull request Mar 26, 2023

[Bug]: plt.errorbar error for all-nan xerr and yerr #25549

Closed

rcomer marked this pull request as draft April 7, 2023 18:43

rcomer force-pushed the safe_first_finite branch from 237e492 to 82a7eac Compare April 8, 2023 12:46

rcomer marked this pull request as ready for review April 8, 2023 12:47

FIX: _safe_first_finite on all non-finite array

f8bc604

rcomer force-pushed the safe_first_finite branch from 82a7eac to f8bc604 Compare April 8, 2023 12:53

rcomer changed the title FIX: _safe_first_finite on all nan array FIX: _safe_first_finite on all non-finite array Apr 8, 2023

oscargus approved these changes Apr 12, 2023

View reviewed changes

jklymak reviewed Apr 18, 2023

View reviewed changes

ksunden approved these changes May 12, 2023

View reviewed changes

ksunden merged commit b61bb0b into matplotlib:main May 12, 2023

meeseeksmachine mentioned this pull request May 12, 2023

Backport PR #25547 on branch v3.7.x (FIX: _safe_first_finite on all non-finite array) #25879

Merged

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request May 12, 2023

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

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 deleted the safe_first_finite branch May 30, 2023 19:47

FIX: `_safe_first_finite` on all non-finite array by rcomer · Pull Request #25547 · matplotlib/matplotlib · GitHub

Repository navigation

FIX: _safe_first_finite on all non-finite array - #25547

FIX: _safe_first_finite on all non-finite array#25547
ksunden merged 1 commit into
matplotlib:mainfrom
rcomer:safe_first_finite

rcomer commented Mar 25, 2023 •
edited
Loading

Uh oh!

Uh oh!

rcomer commented Mar 26, 2023

Uh oh!

oscargus commented Apr 7, 2023

Uh oh!

rcomer commented Apr 7, 2023

Uh oh!

jklymak Apr 18, 2023

Uh oh!

rcomer Apr 18, 2023

Uh oh!

jklymak Apr 18, 2023

Uh oh!

rcomer Apr 19, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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

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

FIX: `_safe_first_finite` on all non-finite array by rcomer · Pull Request #25547 · matplotlib/matplotlib · GitHub

Repository navigation

FIX: _safe_first_finite on all non-finite array - #25547

FIX: _safe_first_finite on all non-finite array#25547ksunden merged 1 commit intomatplotlib:mainmatplotlib/matplotlib:mainfrom rcomer:safe_first_finitercomer/matplotlib:safe_first_finiteCopy head branch name to clipboard

Conversation

rcomer commented Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

rcomer commented Mar 26, 2023

Uh oh!

oscargus commented Apr 7, 2023

Uh oh!

rcomer commented Apr 7, 2023

Uh oh!

jklymak Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

rcomer Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

jklymak Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

rcomer Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FIX: _safe_first_finite on all non-finite array#25547
ksunden merged 1 commit into
matplotlib:mainfrom
rcomer:safe_first_finite

rcomer commented Mar 25, 2023 •
edited
Loading