Simplify parsing of errorbar input. #13124
Conversation
| # special case for empty lists | ||
| if len(err) > 1: | ||
| fe = cbook.safe_first_element(err) | ||
| if len(err) != len(data) or np.size(fe) > 1: |
There was a problem hiding this comment.
the check that len(err) == len(data) is taken care of by safezip, so can be dropped.
lib/matplotlib/axes/_axes.py
Outdated
| # for the (undocumented, but tested) support for (n, 1) arrays. | ||
| ash, bsh = map(np.shape, [a, b]) | ||
| if (len(ash) > 2 and not (len(ash) == 2 and ash[1] == 1) | ||
| or len(bsh) > 2 and not (len(bsh) == 2 and bsh[1] == 1)): |
There was a problem hiding this comment.
I think the expression is wrong. The first part is already False for len(ashape) == 2 so the rhs of the and is never evaluated in that case. Should be
if (len(ashape) > 2 or (len(ashape) == 2 and ashape[1] != 1)
| or len(bsh) > 2 and not (len(bsh) == 2 and bsh[1] == 1)): | ||
| raise ValueError( | ||
| "err must be a scalar or a 1D or (2, n) array-like") | ||
| # Using list comprehensions rather than arrays to preserve units. |
There was a problem hiding this comment.
Would it be worth it to special-case if v and e are arrays to speed up the calculation? Probably not.
|
thanks, handled |
| raise ValueError( | ||
| "err must be a scalar or a 1D or (2, n) array-like") | ||
| # Using list comprehensions rather than arrays to preserve units. | ||
| low = [v - e for v, e in cbook.safezip(data, a)] |
There was a problem hiding this comment.
What units has this been tested with? This is basically the same issue as bar in #12903 so we should make sure unit handling is the same. In particular if v is a datetime, and e a timedelta, I assume this works? What about for pint units? And are we sure this works w/ pandas? Not that I think #12903 checked all those boxes, but I think we should be thinking about how to uniformly handle this case and bar. ie. we now have _convert_dx, and maybe errorbar should use it as well.
There was a problem hiding this comment.
Do you want to first make a PR letting errorbar use _convert_dx? I can rebase this one onto yours after it's merged.
However it's not actually clear to me it's the "same" issue as #12903; in bar() you need to be able to support deunitized widths (because the default, 0.8, is deunitized...) whereas here we can just always assume that the error has the same unit (or a compatible one) as the value.
There was a problem hiding this comment.
I agree they are different and maybe using the same helper doesn't make sense. (I actually think defaulting width=0.8 in bar is ambiguous and a mistake, but...).
Tests with datetime-like obects would be helpful. Not sure categoricals need to be tested (when would the error be a +/- f? )
And we should decide if we should add pint to the tests.
There was a problem hiding this comment.
tbh I don't particularly care about adding tests for units for this PR specifically.
There was a problem hiding this comment.
actually I guess thats fair since you didn't change the algorithm...
| raise ValueError( | ||
| "err must be a scalar or a 1D or (2, n) array-like") | ||
| # Using list comprehensions rather than arrays to preserve units. | ||
| low = [v - e for v, e in cbook.safezip(data, a)] |
There was a problem hiding this comment.
actually I guess thats fair since you didn't change the algorithm...
PR Summary
PR Checklist