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

Simplify parsing of errorbar input. by anntzer · Pull Request #13124 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Simplify parsing of errorbar input. - #13124

Merged
jklymak merged 1 commit into
matplotlib:masterfrom
anntzer:errorbar
Jan 17, 2019
ConversationCommits (1)ChecksFiles changed
Merged

Simplify parsing of errorbar input.#13124
jklymak merged 1 commit into
matplotlib:masterfrom
anntzer:errorbar

Conversation

anntzer commented Jan 6, 2019

Copy link
Copy Markdown
Contributor

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer commented Jan 6, 2019
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py
# 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:

anntzer Jan 6, 2019

Copy link
Copy Markdown
Contributor Author

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

the check that len(err) == len(data) is taken care of by safezip, so can be dropped.

timhoffm reviewed Jan 7, 2019
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py Outdated
Comment thread 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)):

timhoffm Jan 7, 2019

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

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)

anntzer Jan 7, 2019

Copy link
Copy Markdown
Contributor Author

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

duh, indeed

Comment thread lib/matplotlib/axes/_axes.py
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.

timhoffm Jan 7, 2019

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

Would it be worth it to special-case if v and e are arrays to speed up the calculation? Probably not.

anntzer commented Jan 7, 2019

Copy link
Copy Markdown
Contributor Author

thanks, handled

jklymak requested changes Jan 16, 2019
View reviewed changes

jklymak left a comment

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

Seems fine, but blocking on a discussion w/ homogenizing with #12903, which is the same problem for bar.

Comment thread lib/matplotlib/axes/_axes.py
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)]

jklymak Jan 16, 2019

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 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.

anntzer Jan 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

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.

jklymak Jan 17, 2019

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

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.

anntzer Jan 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

tbh I don't particularly care about adding tests for units for this PR specifically.

jklymak Jan 17, 2019

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

actually I guess thats fair since you didn't change the algorithm...

jklymak approved these changes Jan 17, 2019
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py
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)]

jklymak Jan 17, 2019

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

actually I guess thats fair since you didn't change the algorithm...

jklymak merged commit 7f4b044 into matplotlib:master Jan 17, 2019
anntzer deleted the errorbar branch January 17, 2019 19:57
QuLogic added this to the v3.1 milestone Jan 18, 2019
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

jklymak jklymak approved these changes
timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL