Skip to content

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

Closed

Conversation

MaximeLucasSky
Copy link

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
Copy link
Author

MaximeLucasSky commented Dec 27, 2022

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 ?

Copy link

@github-actions github-actions bot left a 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.

@MaximeLucasSky MaximeLucasSky force-pushed the fix-errorbar-crash-on-nan branch from 6b2d985 to c843511 Compare December 27, 2022 11:25
# 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# safe_first_element because getitem is index-first not
# safe_first_finite because getitem is index-first not

Comment on lines +3523 to +3526
except StopIteration:
# this means we found no finite element, fall back to default
# case.
pass
Copy link
Member

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?

Copy link
Member

@timhoffm timhoffm Jan 30, 2023

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.

@jklymak
Copy link
Member

jklymak commented Jan 24, 2023

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

@jklymak jklymak marked this pull request as draft January 24, 2023 17:17
@ebrevdo
Copy link

ebrevdo commented Jan 25, 2023

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
Copy link
Member

rcomer commented Mar 25, 2023

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

@rcomer
Copy link
Member

rcomer commented Mar 25, 2023

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

@rcomer
Copy link
Member

rcomer commented May 13, 2023

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

@rcomer rcomer closed this May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

7 participants