X Tutup
Skip to content

FIX: _safe_first_finite on all non-finite array#25547

Merged
ksunden merged 1 commit intomatplotlib:mainfrom
rcomer:safe_first_finite
May 12, 2023
Merged

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

Conversation

@rcomer
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

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

@jklymak jklymak Apr 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
Copy Markdown
Member Author

@rcomer rcomer Apr 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 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
Copy Markdown
Member

@jklymak jklymak Apr 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
Copy Markdown
Member Author

@rcomer rcomer Apr 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 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

Development

Successfully merging this pull request may close these issues.

[Bug]: ax.errorbar raises for all-nan data on matplotlib 3.6.2 UserWarning thrown when all values are "bad", but not when only some are

4 participants

@rcomer @oscargus @jklymak @ksunden
X Tutup