Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Simplify parsing of errorbar input.#13124
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the check that len(err) == len(data) is taken care of by safezip, so can be dropped.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the expression is wrong. The first part is already False forlen(ashape) == 2 so the rhs of theand is never evaluated in that case. Should be
if (len(ashape) > 2 or (len(ashape) == 2 and ashape[1] != 1)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.
duh, indeed
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it be worth it to special-case if v and e are arrays to speed up the calculation? Probably not.
thanks, handled |
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.
Seems fine, but blocking on a discussion w/ homogenizing with#12903, which is the same problem forbar.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What units has this been tested with? This is basically the same issue asbar 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 forpint 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 andbar. ie. we now have_convert_dx, and maybe errorbar should use it as well.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree they are different and maybe using the same helper doesn't make sense. (I actually think defaultingwidth=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 bea +/- f? )
And we should decide if we should addpint to the tests.
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.
tbh I don't particularly care about adding tests for units for this PR specifically.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
actually I guess thats fair since you didn't change the algorithm...
PR Summary
PR Checklist