NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

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.

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

Jump to bottom

Merged

ksunden merged 1 commit intomatplotlib:mainfromrcomer:safe_first_finite

May 12, 2023

Merged

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

ksunden merged 1 commit intomatplotlib:mainfromrcomer:safe_first_finite

May 12, 2023

Conversation

Copy link

Member

rcomer commentedMar 25, 2023•
edited
Loading

PR Summary

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

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

nan

So this PR reinstates previous behaviour. Currently onmain,_safe_first_finite raisesStopIteration when passed an all-nan array.

Fixes#18294 andfixes#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 (andpytest passes)
[N/A] Documentation is sphinx and numpydoc compliant (the docs shouldbuild 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 indoc/users/next_whats_new/
[N/A] API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
[N/A] Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

rcomer added the PR: bugfixPull 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 thev3.7.2 milestone

Mar 25, 2023

Copy link

MemberAuthor

rcomer commentedMar 26, 2023

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

rcomer mentioned this pull request

Mar 26, 2023

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

Closed

Copy link

Member

oscargus commentedApr 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)
(Changebar to use the new method?)

Copy link

MemberAuthor

rcomer commentedApr 7, 2023

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

rcomer marked this pull request as draft

April 7, 2023 18:43

rcomer force-pushed thesafe_first_finite branch from237e492 to82a7eacCompare

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 thesafe_first_finite branch from82a7eac tof8bc604Compare

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

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

jklymakApr 18, 2023

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

MemberAuthor

rcomerApr 18, 2023

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 in7103779

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

matplotlib/lib/matplotlib/units.py

Lines 182 to 185 in7103779

	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 toStopIteration being thrown for all-nan arrays. Thecommit that introduced the first says it was for zero length objects, and these will still trigger aStopIteration when the logic arriveshere. I'm not sure about the second - shouldunits.Registry.get_converter also be able to handle zero length objects?

Copy link

Member

jklymakApr 18, 2023

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

MemberAuthor

rcomerApr 19, 2023

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 approved these changes

May 12, 2023

View reviewed changes

ksunden merged commitb61bb0b intomatplotlib: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 PRmatplotlib#25547: FIX:_safe_first_finiteon all non-fi…

98b3fa6

…nite array

greglucas added a commit that referenced this pull request

May 13, 2023

Merge pull request#25879from meeseeksmachine/auto-backport-of-pr-25…

fbde951

…547-on-v3.7.xBackport 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

Labels

PR: bugfix

Pull requests that fix identified bugs

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!

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

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

Uh oh!

Conversation

rcomer commentedMar 25, 2023•
edited
Loading

Uh oh!

PR Summary

PR Checklist

Uh oh!

rcomer commentedMar 26, 2023

Uh oh!

oscargus commentedApr 7, 2023

Uh oh!

rcomer commentedApr 7, 2023

Uh oh!

jklymakApr 18, 2023

Choose a reason for hiding this comment

Uh oh!

rcomerApr 18, 2023

Choose a reason for hiding this comment

Uh oh!

jklymakApr 18, 2023

Choose a reason for hiding this comment

Uh oh!

rcomerApr 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Movatterモバイル変換

Uh oh!

FIX:_safe_first_finite on all non-finite array#25547

FIX:_safe_first_finite on all non-finite array#25547

Uh oh!

Conversation

rcomer commentedMar 25, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

rcomer commentedMar 26, 2023

Uh oh!

oscargus commentedApr 7, 2023

Uh oh!

rcomer commentedApr 7, 2023

Uh oh!

jklymakApr 18, 2023

Choose a reason for hiding this comment

Uh oh!

rcomerApr 18, 2023

Choose a reason for hiding this comment

Uh oh!

jklymakApr 18, 2023

Choose a reason for hiding this comment

Uh oh!

rcomerApr 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

rcomer commentedMar 25, 2023•
edited
Loading