BUG: ensure that errorbar does not error on masked negative errors. #29897
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. 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.
c22cac0 to
ae1edf8
Compare
errorbar checks that errors are not negative, but a bit convolutedly, in order to avoid triggering on nan. Unfortunately, the work-around for nan means that possible masks get discarded, and hence passing in a masked error array that has a negative but masked value leads to an exception. This PR solves that by simply combining the test for negative values with the indirect isnan test (err == err), so that if a masked array is passed, the test values are masked and ignored in the check. As a bonus, this also means that astropy's ``Masked`` arrays can now be used -- those refuse to write output of tests to unmasked values since that would discard the mask (which is indeed the underlying problem here).
ae1edf8 to
5c42d71
Compare
|
The underlying (underlying) problem is that |
No, the problem is that when writing directly to a regular array, as happened in p.s. What was a surprise is that |
|
Actually, I guess I misread your question: yes, it is also true that the |
5c42d71 to
ebeb83a
Compare
|
@dstansby - comments addressed (I hope). |
|
Thanks @mhvk! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
…r on masked negative errors.
|
Thanks @mhvk for fixing this! |
…897-on-v3.10.x Backport PR #29897 on branch v3.10.x (BUG: ensure that errorbar does not error on masked negative errors.)
errorbarchecks that errors are not negative, but a bit convolutedly, in order to avoid triggering on nan. Unfortunately, the work-around for nan means that possible masks get discarded, and hence passing in a masked error array that has a negative but masked value leads to an exception. This PR solves that by simply combining the test fornegative values with the indirect isnan test (err == err), so that if a masked array is passed, the test values are masked and ignored in the check.
As a bonus, this also means that astropy's
Maskedarrays can now be used -- those refuse to write output of tests to unmasked values since that would discard the mask (which is indeed the underlying problem here) -- seeastropy/astropy#17996
p.s. I checked that the test case that I added fails on current main. The test case is based on the one for
nanright above - let me know if this is an OK location for it.EDIT: updated to simplify the check without using any storing of outputs. This means we can also continue to use astropy's Quantity for error bars (and just shows how damn careful one has to be, sigh).
EDIT 2: second commit prevents any comparison of
nan < nan, which causesRuntimeErrorthat I did not see locally.PR checklist