FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix errorbar crash on nan values by MaximeLucasSky · Pull Request #24823 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Fix errorbar crash on nan values - #24823

Closed
MaximeLucasSky wants to merge 2 commits into
matplotlib:mainfrom
MaximeLucasSky:fix-errorbar-crash-on-nan
ConversationCommits (2)ChecksFiles changed
Closed

Fix errorbar crash on nan values#24823
MaximeLucasSky wants to merge 2 commits into
matplotlib:mainfrom
MaximeLucasSky:fix-errorbar-crash-on-nan

Conversation

MaximeLucasSky commented Dec 27, 2022

Copy link
Copy Markdown

PR Summary

Fixes issue #24818

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/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

MaximeLucasSky commented Dec 27, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

While it does fix the issue, the added test does not pass, because of a Runtime Warning by Numpy : errorbar calls vline on an all-nan array, which causes a warning when np.nanmax and np.nanmin are called on it.

I am not sure what is the best way to deal with that : should I prevent the call to np.nanmin, prevent the call to vline, catch the warning, or live with the warning ?

github-actions Bot reviewed Dec 27, 2022
View reviewed changes

github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

oscargus reviewed Dec 27, 2022
View reviewed changes
Comment thread lib/matplotlib/tests/test_axes.py Outdated
MaximeLucasSky force-pushed the fix-errorbar-crash-on-nan branch from 6b2d985 to c843511 Compare December 27, 2022 11:25
QuLogic reviewed Dec 29, 2022
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py
# 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

QuLogic Dec 29, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
# safe_first_element because getitem is index-first not
# safe_first_finite because getitem is index-first not

Comment thread lib/matplotlib/axes/_axes.py
Comment on lines +3523 to +3526
except StopIteration:
# this means we found no finite element, fall back to default
# case.
pass

QuLogic Dec 29, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What do you expect to raise this exception? Is it _safe_first_finite?

timhoffm Jan 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jklymak commented Jan 24, 2023

Copy link
Copy Markdown
Member

I've moved to draft, but when updated, we can move back into the queue

jklymak marked this pull request as draft January 24, 2023 17:17

ebrevdo commented Jan 25, 2023

Copy link
Copy Markdown

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.

rcomer commented Mar 25, 2023

Copy link
Copy Markdown
Member

Hi @MaximeLucasSky are you still interested in working on this one?

rcomer commented Mar 25, 2023

Copy link
Copy Markdown
Member

I have proposed a simpler fix for this at #25547.

rcomer commented May 13, 2023

Copy link
Copy Markdown
Member

This is now fixed in #25547. Thank you for your work on this @MaximeLucasSky, but we went for a slightly different approach.

rcomer closed this May 13, 2023
melissawm moved this from Waiting for author to Done in First Time Contributors Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic left review comments
timhoffm timhoffm left review comments
oscargus oscargus left review comments
github-actions[bot] github-actions[bot] left review comments

Assignees

No one assigned

Labels

status: needs revision

Projects

First Time Contributors
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL