DOC: Fixed x, y, docstring in errorbar #8139

Merged
merged 1 commit into from Feb 24, 2017

Conversation

Projects
None yet
7 participants
Contributor

madphysicist commented Feb 24, 2017

Seems to be an obvious omission.

Sorry about the duplicate PR.

[ci skip]

- xerr/yerr : scalar or array-like, shape(n,1) or shape(2,n), optional
- If a scalar number, len(N) array-like object, or an Nx1
+ xerr/yerr : scalar or array-like, shape(N,) or shape(2,N), optional
@tacaswell

tacaswell Feb 24, 2017

Owner

This change gives me pause as there is a bunch of subtle logic in error bar that depends on this shape. Can the exact behavior of (N, ) and (N, 1) arrays be checked before this is merged?

@madphysicist

madphysicist Feb 24, 2017

Contributor

Yes. I did some checks and the docs are 99% correct. I am submitting an issue right now about an inconsistency that I found.

@madphysicist

madphysicist Feb 24, 2017

Contributor

Here is the issue: #8140

@madphysicist

madphysicist Feb 24, 2017

Contributor

The shape is currently checked with len, which means that N, is a more accurate representation than Nx1. I also think that it is less confusing than swapping the dimension that corresponds to N around in the different cases. As a verification, the following works as expected:

>>> import matplotlib.pyplot as plt
>>> plt.ion()
>>> f, a = plt.subplots()
>>> a.errorbar(*[[1, 2, 3, 4, 5, 6]]*2, yerr=list(range(6)), fmt='o')

while the following does not:

>>> a.errorbar(*[[1, 2, 3, 4, 5, 6]]*2, yerr=[[i] for i in range(6)], fmt='o')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jfoxrabi/miniconda3/lib/python3.5/site-packages/matplotlib/__init__.py", line 1892, in inner
    return func(ax, *args, **kwargs)
  File "/home/jfoxrabi/miniconda3/lib/python3.5/site-packages/matplotlib/axes/_axes.py", line 3020, in errorbar
    lower, upper = extract_err(yerr, y)
  File "/home/jfoxrabi/miniconda3/lib/python3.5/site-packages/matplotlib/axes/_axes.py", line 2965, in extract_err
    in cbook.safezip(data, err)]
  File "/home/jfoxrabi/miniconda3/lib/python3.5/site-packages/matplotlib/axes/_axes.py", line 2964, in <listcomp>
    low = [thisx - thiserr for (thisx, thiserr)
TypeError: unsupported operand type(s) for -: 'int' and 'list'

However, this does work:

a.errorbar(*[[1, 2, 3, 4, 5, 6]]*2, yerr=np.array([[i] for i in range(6)]), fmt='o')

So technically, N, 1 was outright wrong for non-array array-likes.
Sorry about that syntax for x, y. I was so fascinated with the fact that it actually worked that I left it in :)

@madphysicist

madphysicist Feb 24, 2017

Contributor

I went ahead and added this pair of examples to issue #8140

Contributor

madphysicist commented Feb 24, 2017

Interesting. Looks like [ci skip] did nothing?

Contributor

afvincent commented Feb 24, 2017

Shouldn't [ci skip] be in the commit message rather than in the PR conversation to have some effect? (I may be wrong, I've only skimmed the AppVeyor and Travis docs.)

@@ -2705,11 +2705,11 @@ def errorbar(self, x, y, yerr=None, xerr=None,
Parameters
----------
- x : scalar
- y : scalar
+ x : scalar or array-like
@anntzer

anntzer Feb 24, 2017

Contributor

I would just say "array-like", scalars is just a special case.

@NelleV

NelleV Feb 24, 2017

Contributor

I'd keep it this way. Array-like refers to iterable, which a scalar isn't.

@anntzer

anntzer Feb 24, 2017

Contributor

scatter, bar, step all use array_like in their docs even though they also accept scalars. Either way we should be consistent.

@madphysicist

madphysicist Feb 24, 2017

Contributor

I think that it should be removed for x and y but retained for errx and erry. x and y are arrays to plot. As @anntzer says, scalars are just a special case that represents a dataset of size 1. For errx and erry, scalars have a somewhat special meaning, which should be emphasized in the docs. Will update.

@madphysicist

madphysicist Feb 24, 2017

Contributor

Another argument in @anntzer's favor is that the scalar case is explicitly dealt with in the sentence above the parameters section.

@NelleV

NelleV Feb 24, 2017

Contributor

FYI, this is not what array-like means in any other scientific projects in the community. The fact that scatter, bar, step use array-like to mean scalar or array-like is probably just a mistake.

@madphysicist

madphysicist Feb 24, 2017

Contributor

I have seen it used that way pretty consistently in numpy, where a scalar gets converted to an array of shape (0,) when necessary.

@anntzer

anntzer Feb 24, 2017

Contributor

e.g. np.sum:

    Parameters
    ----------
    a : array_like
        Elements to sum.
In [1]: np.sum(1)
Out[1]: 1
@NelleV

NelleV Feb 24, 2017

Contributor

As I said, I really don't care as I consider Matplotlib's loose input format harmful and not useful for the project, but this convention is not only unclear, but not followed by major scientific projects (scipy, sklearn, skimage, sympy pandas and even Matplotlib).

It boils down whether you want or not to document the fact that errorbar takes scalars as input. I am, once again, totally fine with not documenting it, but let's not pretend that the current docstring documents this in any meaningful and clear way.

@madphysicist

madphysicist Feb 24, 2017

Contributor

The previous sentence reads:

x, y, xerr, and yerr can all be scalars, which plots a single error bar at x, y.

I think that should be enough for most users.

@anntzer

anntzer Feb 24, 2017

Contributor

I think that's good enough.

FWIW git-grepping the codebase of scipy and skimage yields no result for ... or array-like or array-like or ... (where ... refers to "scalar", "float", or some similar term); meanwhile at least some functions that are documented to take an "array-like" as argument can also take a scalar.

scikit-learn is different because it documents the shape of the required input nearly every time it documents an input as array-like, so scalars are explicitly excluded by that shape info.

sympy uses the word array-like exactly twice in its docs (in the sense of not including scalars).

pandas seems to use the word array-like in the sense of not including scalars as well.

So overall I wouldn't say there's any global agreement.

@madphysicist

madphysicist Feb 24, 2017

Contributor

Since this thread is based on an outdated diff, I went ahead and changed back to scalar or array-like. I think that is a reasonable compromise that does not sacrifice clarity, which is really the main objective here (more so than consistency in my opinion).

Contributor

NelleV commented Feb 24, 2017

The CI should not be skipped anyways, so it is a good thing that it didn't work.

Contributor

NelleV commented Feb 24, 2017

I really don't care if the fact that these functions take scalars in addition of array-like so the patch is fine with me, but just to be clear, there is no way that anyone will understand scalars can be provided to these functions. In addition, the consistency argument brought up by @anntzer doesn't hold considering git grep array-like returns:

lib/matplotlib/axes/_axes.py:        or 2D array-like object, with each row corresponding to a row or column
lib/matplotlib/axes/_axes.py:          A float or array-like containing floats.
lib/matplotlib/axes/_axes.py:          A float or array-like containing floats.
lib/matplotlib/axes/_axes.py:          A float or array-like containing floats.
lib/matplotlib/axes/_axes.py:        value is given, that value is applied to all lines.  If an array-like
lib/matplotlib/axes/_axes.py:        width : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        bottom : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        color : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        edgecolor : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        linewidth : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        tick_label : string or array-like, optional
lib/matplotlib/axes/_axes.py:        xerr : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        yerr : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        ecolor : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        bottom : scalar or array-like
lib/matplotlib/axes/_axes.py:        width : scalar or array-like
lib/matplotlib/axes/_axes.py:        color : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        edgecolor : scalar or array-like, optional
lib/matplotlib/axes/_axes.py:        linewidth : scalar or array-like, optional, default: None
lib/matplotlib/axes/_axes.py:        tick_label : string or array-like, optional, default: None
lib/matplotlib/axes/_axes.py:        xerr : scalar or array-like, optional, default: None
lib/matplotlib/axes/_axes.py:        yerr : scalar or array-like, optional, default: None
lib/matplotlib/axes/_axes.py:        ecolor : scalar or array-like, optional, default: None
lib/matplotlib/axes/_axes.py:        xerr/yerr : scalar or array-like, shape(n,1) or shape(2,n), optional
lib/matplotlib/axes/_axes.py:            If a scalar number, len(N) array-like object, or an Nx1
lib/matplotlib/axes/_axes.py:            array-like object, errorbars are drawn at +/-value relative
lib/matplotlib/axes/_axes.py:                                     "or 2xN array-like ]")
lib/matplotlib/axes/_axes.py:        usermedians : array-like, optional

The reason I don't care whether this feature is documented properly is that I believe Matplotlib is too flexible in its input type, and the scalar input can as easily be a 1D numpy vector. But if the goal is to document properly this feature or be consistent, the patch does not achieve this goal.

I have no clue whether the current policy of Matplotlib is too accurately document all arguments and input format.

Contributor

madphysicist commented Feb 24, 2017

I don't mind changing the other functions to mention scalars too. Slight overkill on the docs may be better than underkill, especially for new users. Basically, I will do whatever once there is some sort of consensus.

Contributor

anntzer commented Feb 24, 2017

Well we clearly have a mix of both, you can also git grep for : array-like and get a bunch of results (but then you'd have to check for whether each of these cases actually support scalars as well). (Plus, ultimately I'd say it is up to the numpy project (if anyone...) to define what array-like means; if they include scalars in them then we should do the same.)

But I agree with @NelleV that overall matplotlib is too flexible with its inputs. I'd say if a user looks at the docs and thinks he has to write errorbar([1], [2], [3]) instead of errorbar(1, 2, 3), it's really not an issue.

Anyways, I thought the point of this PR was more regarding the Nx1 array-like part of the docstring.

Contributor

madphysicist commented Feb 24, 2017

Actually it was more because the docs only said scalar and did not mention arrays at all. I think that we can all agree that that's just wrong. The Nx1 thing is partially wrong, but I opened issue #8140 to deal with that.

@madphysicist @madphysicist madphysicist DOC: Fixed x, y, docstring in errorbar
Seems to be an obvious omission
652224c
@tacaswell

Independent of a wider standardization of type specifications, this is clearly an improvement!

tacaswell changed the title from DOC: Fixed x, y, docstring in errorbar to [MRG+1] DOC: Fixed x, y, docstring in errorbar Feb 24, 2017

@NelleV

NelleV approved these changes Feb 24, 2017

NelleV changed the title from [MRG+1] DOC: Fixed x, y, docstring in errorbar to [MRG+2] DOC: Fixed x, y, docstring in errorbar Feb 24, 2017

@NelleV NelleV merged commit ba418fd into matplotlib:master Feb 24, 2017

3 of 5 checks passed

codecov/project/library 57.84% (-0.06%) compared to 7220afd
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
codecov/patch Coverage not affected when comparing 7220afd...652224c
Details
codecov/project/tests 98.56% (target 97.9%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

madphysicist deleted the madphysicist:patch-1 branch Feb 27, 2017

@dstansby dstansby added a commit that referenced this pull request Mar 1, 2017

@NelleV @dstansby NelleV + dstansby Merge pull request #8139 from madphysicist/patch-1
DOC: Fixed x, y, docstring in errorbar
e01596e
Contributor

dstansby commented Mar 1, 2017

Backported to 2.0.0-doc via e01596e

Owner

tacaswell commented Mar 1, 2017

This should not have got back to 2.0.0-doc, it changes a source (.py) file, not just rst.

Owner

tacaswell commented Mar 1, 2017

I am thinking about what, if anything we should do about this though.

QuLogic changed the title from [MRG+2] DOC: Fixed x, y, docstring in errorbar to DOC: Fixed x, y, docstring in errorbar Mar 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment