FIX: _safe_first_finite on all non-finite array
#25547
Conversation
|
I see that a different approach was taken to handle the change in behaviour for |
|
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:
|
|
Thanks @oscargus your suggestions make sense to me. I’ll put this in draft until I have time to implement them. |
237e492 to
82a7eac
Compare
82a7eac to
f8bc604
Compare
_safe_first_finite on all nan array_safe_first_finite on all non-finite array
| 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): |
There was a problem hiding this comment.
I think there a few other places where StopIteration is try/excepted for _safe_first_element? Did you check all those?
There was a problem hiding this comment.
I hadn't, but I have now. I only found these two:
matplotlib/lib/matplotlib/dates.py
Lines 1828 to 1831 in 7103779
matplotlib/lib/matplotlib/units.py
Lines 182 to 185 in 7103779
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
…547-on-v3.7.x Backport PR #25547 on branch v3.7.x (FIX: `_safe_first_finite` on all non-finite array)
PR Summary
This function used to be named
_safe_first_non_none(#23751), and at v3.6.0 we haveSo this PR reinstates previous behaviour. Currently on
main,_safe_first_finiteraisesStopIterationwhen 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
PR Checklist
< 8000 p dir="auto">Documentation and Testspytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst