-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix errorbar crash on nan values #24823
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix errorbar crash on nan values #24823
Conversation
While it does fix the issue, the added test does not pass, because of a Runtime Warning by Numpy : I am not sure what is the best way to deal with that : should I prevent the call to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
6b2d985
to
c843511
Compare
# and it is not empty | ||
len(err) > 0 and | ||
# and the first element is an array sub-class use | ||
# safe_first_element because getitem is index-first not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# safe_first_element because getitem is index-first not | |
# safe_first_finite because getitem is index-first not |
except StopIteration: | ||
# this means we found no finite element, fall back to default | ||
# case. | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you expect to raise this exception? Is it _safe_first_finite
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we raise a more telling error in _safe_first_finite
? I think a ValueError subclass would make sense here to allow a more specific catching.
I've moved to draft, but when updated, we can move back into the queue |
I hit this issue recently and am glad to have found work on the fix. Looking forward to updates; but LMK if I can help. |
Hi @MaximeLucasSky are you still interested in working on this one? |
I have proposed a simpler fix for this at #25547. |
This is now fixed in #25547. Thank you for your work on this @MaximeLucasSky, but we went for a slightly different approach. |
PR Summary
Fixes issue #24818
PR Checklist
Documentation and Tests
pytest
passes)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.rst
ornext_api_changes/README.rst