FIX: eventplot 'colors' kwarg (#8193) #8204

afvincent · 2017-03-06T09:15:29Z

Strings longer than a single character should now be OK as the colors parameter for matplotlib.axes.eventplot. NB: before this PR, colors='rgb' was OK as long as the length of positions was equal to 3 too. As I do not know if “it is not bug, it is a feature!”™, the current PR keeps this behavior.

I added an image test. I now realize that it may be a bit overkill… Should I rewrite the test to be a non-image one?

Besides I did some overhaul of the docstrings of eventplot and EventCollection (BTW this PR should fix #7917 too) that seemed too be quite inexact, especially about what could be given as colors parameters For example, any list of valid colors was OK, eventhough the docstrings were asking for RGBA values only… (Being there, I tried to numpify the docstrings.)

Pinging @anntzer for review as you opened #7917 and gave advices on #8193 😇 .

anntzer · 2017-03-06T18:05:11Z

lib/matplotlib/axes/_axes.py

-        Plot parallel lines at the given positions.  positions should be a 1D
-        or 2D array-like object, with each row corresponding to a row or column
-        of lines.
+        Plot identical parallel lines at the given positions. positions should


Keep a single summary line.
Mark arguments as *such*. (See "documenting matplotlib" entry in the faq.)

If I did not forget any utterance, it should be OK now with c3d7e8d.

anntzer · 2017-03-06T18:05:52Z

lib/matplotlib/axes/_axes.py


        This type of plot is commonly used in neuroscience for representing
-        neural events, where it is commonly called a spike raster, dot raster,
+        neural events, where it is usually called a spike raster, dot raster,


"commonly" looks more idiomatic but I am not a native speaker either...

Maybe some English guru as @QuLogic might be able to enlighten us about this :). (I was just trying to avoid a repetition with the line just above.)

anntzer · 2017-03-06T18:06:31Z

lib/matplotlib/axes/_axes.py

-          'horizontal' : the lines will be vertical and arranged in rows
-          'vertical' : lines will be horizontal and arranged in columns
+        Parameters
+        ==========


Use dashes (-) for underlining.

Should now be OK.

anntzer · 2017-03-06T19:31:55Z

lib/matplotlib/axes/_axes.py

+        linestyles : str or tuple or a sequence of such values, optional.
+            Default is 'solid'. Valid strings are [ 'solid' | 'dashed' |
+            'dashdot' | 'dotted' | '-' | '--' | '-.' | ':' ]. Dash tuples
+            have to follow::


"have to be of the form"?

Fixed by fc212ee.

anntzer · 2017-03-06T19:33:50Z

lib/matplotlib/collections.py

-            a sequence of numerical values or a 1D numpy array.  Can be None
+        Parameters
+        ----------
+        positions : 1D array-like object or None.


Looks like the previous implementation did not support passing None as input (despite being documented that it does). If that is correct I would fix the docstring to say it's not supported (I don't see why it should).

I was going into the other direction :/ (based on the fact that LineCollection accepts None). Your suggestion may be better actually:

changing the docstring is some kind of API break but as it was raising an error anyway…

accepting None is not without some side-effects on eventplot, which I tried to mitigate by making eventplot(None) equivalent to eventplot([]), but on second thought the API break may even be worse than the previous solution.

if it was not working before it's not really an API break...

Yes you're right, I will revert my changes toward your idea of just fixing the docstring.

Done (reverted my changes and instead simply fixed the relevant docstring) by fc212ee.

anntzer · 2017-03-06T19:34:24Z

lib/matplotlib/collections.py

+        linestyle : str or tuple, optional. Default is 'solid'.
+            If it is a string, it has to be among [ 'solid' | 'dashed' |
+            'dashdot' | 'dotted' | '-' | '--' | '-.' | ':' ]. If it is a dash
+            tuple, it has to be of the form::


same as above.

Fixed by fc212ee.

anntzer · 2017-03-06T19:36:23Z

lib/matplotlib/collections.py

-            raise ValueError('if positions is an ndarry it cannot have '
-                             'dimensionality great than 1 ')
+            raise ValueError('if positions is an ndarray it cannot have '
+                             'dimensionality strictly greater than 1 ')


"greater than 1" (In English, greater = "strictly greater").

Oh! I did not know that 🐑

https://en.wikipedia.org/wiki/Inequality_(mathematics)
https://en.wikipedia.org/wiki/Sign_(mathematics)#non-negative_and_non-positive
"Because zero is neither positive nor negative (in most countries)" I guess we just like being different in France...

Fixed by fc212ee.

anntzer · 2017-03-06T19:37:22Z

lib/matplotlib/collections.py


        .. plot:: mpl_examples/pylab_examples/eventcollection_demo.py
        """

        segment = (lineoffset + linelength / 2.,
                   lineoffset - linelength / 2.)
-        if len(positions) == 0:
+        if positions is None or len(positions) == 0:


would just write positions = np.asarray(positions) (or np.atleast_1d(positions) if we want to support scalars)

(and then you know for sure it has an ndim attribute)

The docstring does not say that one should expect scalars to be supported, and the other collections classes does not seem to support it either. So I would say => np.asarray.

Now using positions = np.asarray(positions) since fc212ee.

Reverted on a @tacaswell's comment.

anntzer · 2017-03-06T19:37:56Z

lib/matplotlib/collections.py

-            raise ValueError('if positions is an ndarry it cannot have '
-                             'dimensionality great than 1 ')
+            raise ValueError('if positions is an ndarray it cannot have '
+                             'dimensionality strictly greater than 1 ')
        elif (orientation is None or orientation.lower() == 'none' or
              orientation.lower() == 'horizontal'):
            positions.sort()


Do we really need to sort the positions?

I do not totally understand how it works, be it looks like it is needed by how the “offsets” are handled in LineCollection. In particular, the _add_offsets seems to be sensitive to the ordering of the segments.

For the fun I tried to not sorting the positions: the tests do not pass anymore. So I would be inclined to conclude that indeed we need to sort the positions ^^…

NelleV

I've got a couple of minor comments, and one major comment: we decided a while back to drop docstring interpolation. Can you remove it and add a reference to the line collection arguments instead?

Else, it looks good.

NelleV · 2017-03-06T20:27:02Z

lib/matplotlib/axes/_axes.py

-        Plot parallel lines at the given positions.  positions should be a 1D
-        or 2D array-like object, with each row corresponding to a row or column
-        of lines.
+        *positions* should be a 1D or 2D array-like object, with each row


I think we had a consensus to drop the "*".

See discussion starting here: #8172 (comment)
ref http://matplotlib.org/devdocs/devel/documenting_mpl.html#formatting
ref https://docs.python.org/devguide/documenting.html#id3

Let's keep the asterisks unless there is agreement to switch to double backquotes (which I would not favor). If we go with such a switch the documenting matplotlib guide should be updated first.

Currently still using *. I will change it if a consensus emerges that it is not the proper markup to use (i.e. if the dev guidelines are updated :) ).

NelleV · 2017-03-06T20:28:05Z

lib/matplotlib/collections.py

-    an axis, such as time or length.  Events do not have an amplitude.  They
-    are displayed as v
+    The events are a 1-dimensional array, usually the position of something
+    along an axis, such as time or length.  The do not have an amplitude and


There is a small typo: The -> They

Fixed by 6713a76.
Edit: Fixed the reference to the commit…

NelleV · 2017-03-06T20:29:29Z

lib/matplotlib/collections.py

-        :attr:`~matplotlib.cm.ScalarMappable._A` is not None (i.e., a call to
-        :meth:`~matplotlib.cm.ScalarMappable.set_array` has been made), at
-        draw time a call to scalar mappable will be made to set the colors.
+        %(LineCollection)s


We do not use string interpolation in docstring anymore. This line should be removed and replace with a reference to the line collection arguments.

@NelleV From a quick look at pyplot.py, there are several ways of refering to additional kwargs. What would you prefer between

The keyword arguments *kwargs*, if any, are :class:`~matplotlib.collections.LineCollection` properties.

and

kwargs : optional Other keyword arguments are line collection properties. See :class:`~matplotlib.collections.LineCollection` for a list of the valid properties.

? For the latter, I would move it at the end of the parameter list.

6713a76 replaces it with the second solution that I mentioned. If it is too verbose, I could change it for the first proposition and squash/rebase the commits.

Edit: Fixed the reference to the commit…

afvincent · 2017-03-06T20:33:37Z

@NelleV I just saw your comments. I cannot address them right now but I will address them ASAP :).

afvincent · 2017-03-12T16:09:13Z

@NelleV and @anntzer thank you both for the previous reviews. Most of your comments should have been adressed with the last two commits (fc212ee and 6713a76). The ones that haven't been fixed are the ones on which you do not agree both ^^, or that were more a question than a fix to perform.

Edit: typos…

tacaswell · 2017-03-12T20:31:25Z

lib/matplotlib/collections.py


        .. plot:: mpl_examples/pylab_examples/eventcollection_demo.py
        """

        segment = (lineoffset + linelength / 2.,
                   lineoffset - linelength / 2.)
+        positions = np.asarray(positions)


This can strip off unit information, can you remove this set of changes?

tacaswell

Don't cast to array to avoid breaking unit handling.

QuLogic

Docstring stuff.

QuLogic · 2017-03-12T20:33:51Z

lib/matplotlib/axes/_axes.py

-          'vertical' : lines will be horizontal and arranged in columns
+        Parameters
+        ----------
+        positions : 1D or 2D array-like object.


The type doesn't require trailing punctuation.

If I did not forget any, I removed trailing punctuation for all the paramater entries.

QuLogic · 2017-03-12T20:35:07Z

lib/matplotlib/axes/_axes.py


-        *lineoffsets* :
-          A float or array-like containing floats.
+        orientation : str [ 'horizontal' (default) | 'vertical' ], optional.


orientation : {'horizontal', 'vertical'}, optional is the way to specify a choice of values with numpydoc (the first value is the default.)

QuLogic · 2017-03-12T20:37:53Z

lib/matplotlib/axes/_axes.py

-        *lineoffsets* :
-          A float or array-like containing floats.
+        orientation : str [ 'horizontal' (default) | 'vertical' ], optional.
+            'horizontal' : the lines will be vertical and arranged in rows.


This is confusing because it says horizontal is vertical and vertical is horizontal. I would swap the order of the descriptions so that things that are the same come next to each other, i.e., 'horizontal': the lines are arranged horizontally in rows, and are vertical.

Also, these are not bullet points or separate paragraphs, so keep that in mind; both lines will appear as one paragraph in the rendered docs.

Yep, I agree it was confusing (I was surprised too the first time I read it…). So, changed accordingly to your suggestion. And the 2 values should now be displayed as a 2-entry bullet list.

QuLogic · 2017-03-12T20:40:06Z

lib/matplotlib/axes/_axes.py

-          A float or array-like containing floats.
+        linelengths : scalar or sequence of scalars, optional. Default is 1.
+            The total height of the lines (i.e. the lines stretches from
+            ``lineoffset + linelength/2`` to ``lineoffset - linelength/2``).


Negative before positive.

QuLogic · 2017-03-12T20:40:43Z

lib/matplotlib/axes/_axes.py

-          must be a sequence of RGBA tuples (e.g., arbitrary color
-          strings, etc, not allowed) or a list of such sequences
+        linewidths : scalar, scalar sequence or None, optional. Default: None.
+            If it is None, defaults to its rcParams setting.


Not actually a description of what it is.

I agree but it was like this before, so I did wanted to change it. I added a sentence (that may be obvious though):

The line width(s) of the event lines, in points.

I also added something similar for the colors parameter entry just after.

QuLogic · 2017-03-12T20:46:37Z

lib/matplotlib/collections.py

-            a single numerical value
+        linelength : scalar, optional. Default is 1.
+            The total height of the marker (i.e. the marker stretches from
+            ``lineoffset + linelength/2`` to ``lineoffset - linelength/2``).


Negative to positive.

QuLogic · 2017-03-12T20:47:47Z

lib/matplotlib/collections.py

-            1 or 2
+        linestyle : str or tuple, optional. Default is 'solid'.
+            Valid strings are [ 'solid' | 'dashed' | 'dashdot' | 'dotted' |
+            '-' | '--' | '-.' | ':' ]. Dash tuples have to be of the form::


I'd make this a list instead.

@QuLogic I am not sure I understand what you mean. You would write something like:

linestyle : str or tuple, optional. Default is 'solid'. Valid strings are ['solid', 'dashed', 'dashdot', 'dotted', '-', '--', '-.', ':']. Dash tuples should be of the form::

or

linestyle : ['solid', 'dashed', 'dashdot', 'dotted', '-', '--', '-.', ':', tuple], optional Dash tuples should be of the form::

instead?

The former.

Ok thanks! Done (in _axes.eventplot too).

QuLogic · 2017-03-12T20:48:11Z

lib/matplotlib/collections.py


-        *pickradius* is the tolerance for mouse clicks picking a line.
-        The default is 5 pt.
+        antialiased : [None (default) | 1 | 2], optional.


{None, 1, 2}

QuLogic · 2017-03-12T20:48:26Z

lib/matplotlib/collections.py

-        :attr:`~matplotlib.cm.ScalarMappable._A` is not None (i.e., a call to
-        :meth:`~matplotlib.cm.ScalarMappable.set_array` has been made), at
-        draw time a call to scalar mappable will be made to set the colors.
+        kwargs : optional


QuLogic · 2017-03-12T20:50:15Z

lib/matplotlib/axes/_axes.py

-        *linelengths* :
-          A float or array-like containing floats.
+        lineoffsets : scalar or sequence of scalars, optional. Default is 1.
+            The offset of the center of the lines from the origin.


I think this needs a clarification of which way. If it's horizontal, then is it an offset horizontally or vertically?

I tried to make the statement a bit more precise:

The offset of the center of the lines from the origin, in the orthogonal direction to *orientation*.

Changes asked have been applied.

afvincent · 2017-03-20T00:18:17Z

The comments made by @QuLogic and @tacaswell should now have been adressed :). I had to rebase to be able to build the docs under Python 2.7 (my branch was done before the recent fix that made docs to build again with 2.7…). Being there, I squashed some tiny commits.

I think there is still (at least) one question: “In the present case, to test (only) the colors parameters, is a image test a nuke or not?”…

QuLogic

If you could write it in a way that doesn't require an image, that's probably better since it'll save on space and I/O and stuff.

QuLogic · 2017-03-20T01:06:30Z

lib/matplotlib/axes/_axes.py

-          A float or array-like containing floats.
+        lineoffsets : scalar or sequence of scalars, optional, default: 1
+            The offset of the center of the lines from the origin, in the
+            orthogonal direction to *orientation*.


orthogonal direction -> direction orthogonal

QuLogic · 2017-03-20T01:14:05Z

lib/matplotlib/collections.py

-            lineoffset-linelength/2).  Defaults to 1
+        lineoffset : scalar, optional, default: 0
+            The offset of the center of the markers from the origin, in the
+            orthogonal direction to *orientation*.


orthogonal direction -> direction orthogonal

QuLogic · 2017-03-20T01:17:50Z

lib/matplotlib/tests/test_axes.py

+    axs[1, 1].eventplot(data, colors='rgbk')
+
+    for ax in axs.flat:
+        ax.set_xlim(-0.1, 3.1)  # avoid having some events on the axes edges


You could pass style='default' and then get automatic margins.

I switched for a non-picture test ;)

afvincent · 2017-03-20T07:53:17Z

Ok, normally the new test should be equivalent to the previous one, but is not based on image_comparison anymore :). I guess I will have to rebase once it will be validated, to remove the previous test image?

afvincent · 2017-03-20T09:11:36Z

Of course there are some PEP8 issues… And I wanted to make @anntzer happy with np.broadcast_to but it seems that it is actually only in Numpy 1.10+. I'll try to fix this this tonight.

anntzer · 2017-03-20T15:43:23Z

https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook/_backports.py#L41

afvincent · 2017-03-20T23:56:23Z

Fixed the PEP8 issue and now use the cbook._backports version of broadcast_to (thanks again @anntzer :) ).

Re-rebased, and squashed everything into 3 (more or less) self-contained commits. Doing this, the picture test and the associated PNG file were removed, as well as the unfortunate idea of casting everything to np.array on @tacaswell's demand (subtle hidden ping ^^).

Fingers crossed that Travis will be happy this time (at least locally, tests seem to be ok and the docs can be built).

QuLogic · 2017-03-21T03:52:45Z

lib/matplotlib/axes/_axes.py

+        For *linelengths*, *linewidths*, *colors*, and *linestyles*, if only
+        a single value is given, that value is applied to all lines.  If an
+        array-like is given, it must have the same length as *positions*, and
+        each value will be applied to the corresponding row or column of


Maybe just say "row of the array" to avoid this ambiguous conjunction (IIUC, it's always the row of the array.)

Thanks @QuLogic . I changed the phrasing as you suggested (if I understood correctly what you were saying):

For *linelengths*, *linewidths*, *colors*, and *linestyles*, if only a single value is given, that value is applied to all lines. If an array-like is given, it must have the same length as *positions*, and each row of the array will be applied to the corresponding row or column of events.

and then rebased and squashed.

No, I was actually referring to the events at the end of the sentence. There isn't any row of the linelengths/linewidths/colors/linestyles because they're all 1D.

Indeed… It seems that I was I was tired this morning and I mixed this arrays with positions in my head ^^. Sorry, I will fix it ASAP.

Hopefully it is now fixed as you suggested by c0c6bc9 ^^.

Yep, that looks about right.

…consistent with its behavior)

NelleV · 2017-03-21T15:47:18Z

@anntzer @tacaswell can you review this patch again?

anntzer · 2017-03-21T16:43:46Z

lib/matplotlib/collections.py

-            raise ValueError('if positions is an ndarry it cannot have '
-                             'dimensionality great than 1 ')
+            raise ValueError('positions cannot have a dimensionality greater '
+                             'than 1 (in the ndarray sense)')


cannot be an array with more than one dimension.

Done (c0c6bc9).

anntzer · 2017-03-21T16:44:22Z

lib/matplotlib/collections.py

-        *pickradius* is the tolerance for mouse clicks picking a line.
-        The default is 5 pt.
+        antialiased : {None, 1, 2}, optional
+            If it is None, defaults to its rcParams setting, in sequence form.


can antialiased really be a sequence?

TBH I have no idea of what this parameter is doing except that it has to do with AA. So I tried to avoid changing the original sentence as much as possible.

From a very quick look at other docstrings in colllections, a sequence form is mentioned very often for the parameters antialiased(s). However, it is not totally consistent with this one here, as most of them state that is has to be boolean or sequence of boolean (or 0 and 1 values)… I'll try to have a deeper look today.

tacaswell · 2017-07-11T04:05:31Z

Closing in favor of #8861 (which is just these commits rebased)

@afvincent If you want to take this back over, close my PR or just push to my branch there.

afvincent · 2017-07-11T13:50:46Z

@tacaswell No problem with you taking over this PR. I will not have time for it during a little while anyway (just moved in to the US and starting a new postdoc => a bunch of things need to be handled first ^^)

afvincent added API: consistency Documentation labels Mar 6, 2017

afvincent added this to the 2.0.1 (next bug fix release) milestone Mar 6, 2017

afvincent requested a review from anntzer March 6, 2017 09:15

anntzer requested changes Mar 6, 2017

View reviewed changes

NelleV previously requested changes Mar 6, 2017

View reviewed changes

afvincent added the not a blocker label Mar 8, 2017

tacaswell reviewed Mar 12, 2017

View reviewed changes

tacaswell requested changes Mar 12, 2017

View reviewed changes

QuLogic reviewed Mar 12, 2017

View reviewed changes

afvincent force-pushed the fix_eventplot_colors_kwarg branch from 6713a76 to e70e5fd Compare March 20, 2017 00:10

QuLogic reviewed Mar 20, 2017

View reviewed changes

afvincent added 2 commits March 20, 2017 23:58

Naive fix for issue 8193

e4fe239

Add a non-picture test dedicated to the 'colors' parameter

c03df1f

afvincent force-pushed the fix_eventplot_colors_kwarg branch from cbdec92 to 93d9e2e Compare March 20, 2017 23:45

QuLogic approved these changes Mar 21, 2017

View reviewed changes

eventplot and EventCollection docstrings overhaul (+ make the latter …

c49a4c8

…consistent with its behavior)

afvincent force-pushed the fix_eventplot_colors_kwarg branch from 93d9e2e to c49a4c8 Compare March 21, 2017 06:56

NelleV changed the title ~~FIX: eventplot 'colors' kwarg (#8193)~~ [MRG+1] FIX: eventplot 'colors' kwarg (#8193) Mar 21, 2017

anntzer requested changes Mar 21, 2017

View reviewed changes

Fix the phrasing of some sentences.

c0c6bc9

QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017

tacaswell modified the milestones: 2.1 (next point release), 2.0.3 (next bug fix release) Jul 11, 2017

tacaswell mentioned this pull request Jul 11, 2017

Fix eventplot colors kwarg #8861

Merged

6 tasks

tacaswell closed this Jul 11, 2017

dstansby added the status: duplicate label Jul 11, 2017

QuLogic changed the title ~~[MRG+1] FIX: eventplot 'colors' kwarg (#8193)~~ FIX: eventplot 'colors' kwarg (#8193) Oct 16, 2017

Uh oh!

FIX: eventplot 'colors' kwarg (#8193) #8204

FIX: eventplot 'colors' kwarg (#8193) #8204

Uh oh!

Conversation

afvincent commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afvincent Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afvincent Mar 12, 2017 •

edited

Loading

afvincent Mar 12, 2017 •

edited

Loading

afvincent commented Mar 6, 2017 •

edited

Loading

afvincent commented Mar 12, 2017 •

edited

Loading

QuLogic Mar 12, 2017 •

edited

Loading