Skip to content

eventplot throws exception when using color different than one of {'b', 'g', 'r', 'c', 'm', 'y', 'k', 'w'} #8193

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aweinstein opened this issue Mar 3, 2017 · 13 comments
Milestone

Comments

@aweinstein
Copy link

Bug report

The user guide (http://matplotlib.org/users/colors.html) specifies different ways to specify the color parameter. However, eventplot throws an exception when using a color different than the one in {'b', 'g', 'r', 'c', 'm', 'y', 'k', 'w'}, such as color='C0' or color='0.5'.

Code for reproduction

plt.eventplot([1,2], color='C0')

Actual outcome

Traceback (most recent call last):
  File "<ipython-input-15-6ce26cd5e5a1>", line 1, in <module>
    plt.eventplot([1,2], color='C0')
  File "/home/ajw/.local/anaconda/lib/python3.6/site-packages/matplotlib/pyplot.py", line 2956, in eventplot
    linestyles=linestyles, data=data, **kwargs)
  File "/home/ajw/.local/anaconda/lib/python3.6/site-packages/matplotlib/__init__.py", line 1892, in inner
    return func(ax, *args, **kwargs)
  File "/home/ajw/.local/anaconda/lib/python3.6/site-packages/matplotlib/axes/_axes.py", line 1219, in eventplot
    raise ValueError('colors and positions are unequal sized '
ValueError: colors and positions are unequal sized sequences

Matplotlib version

  • Matplotlib version 2.2.0, Python version 3.6, running on Linux
  • Anaconda 4.3.0
@afvincent
Copy link
Contributor

You are lucky: the Matplotlib version that you are using is from the future! 2.0.1 is not even out yet ;).

From a quick look at the code, I would say that this reveals some kind of inconsistency in the API. The docstring says that only an RGBA (or a list of RGBA) value(s) should be provided to colors (btw, the kwarg is a plural in the docstring):

*colors*
  must be a sequence of RGBA tuples (e.g., arbitrary color
  strings, etc, not allowed) or a list of such sequences

The same is present in the docstring of LineCollection. However, the internal code does somehow manages to translate single-character color names to RGBA values.

I suspect that in the case of passing a string longer than a single character, each character is considered as a color, which causes two issues:

  • first len('C0') is 1, while you only have one type of events => this causes the exception you reported;
  • even without this first problem, 'C' in color='C0' or '.' and '5' in color='0.5' are not valid single string characters.

@afvincent
Copy link
Contributor

A workaround may be to manually handle the conversion into RGBA:

from matplotlib.colors import to_rgba_array

plt.eventplot([1,2], colors=to_rgba_array('C0'))  # color= is also possible

@aweinstein
Copy link
Author

Thanks for the answer @afvincent. My mistake with Matplotlib version. It is 2.0.0.

And also sorry with the confusion between color and colors. But as you said, both seems to behaves in the same way.

@afvincent
Copy link
Contributor

@aweinstein No worries :).

Funnily color appears in eventplotdocstring as a possible kwarg of LineCollection but only colorsis listed in LineCollection docstring…

@afvincent
Copy link
Contributor

To other devs

I wonder if simply replacing this line in eventplot by

if isinstance(colors, six.string_types) or not iterable(colors):

would fix the issue (strings are iterable, so currently the next line will be skipped and we will not pass a list of valid color names to LineCollection...).

Besides, I do not understand why the docstrings precise that only RGBA values should be given. Under the hood, it seems to me that everything will be OK we a list of any type of valid colors (RGBA values, single characters, full string or Cn names), as in the end everyhting goes through to_rgba_array in LineCollection.

@anntzer
Copy link
Contributor

anntzer commented Mar 4, 2017

I would do something like:

colors = _backports.broadcast_to(mcolors.to_rgba_array(colors), (len(positions), 4))

@QuLogic
Copy link
Member

QuLogic commented Mar 4, 2017

Err, documentation issues aside, a much simpler workaround is to pass a list: plt.eventplot([1, 2], color=['C0']) works fine.

@afvincent
Copy link
Contributor

@anntzer What is the advantage of using _backports.broadcast_to instead of the current way to extend a one-element list to a list with len(positions) elements if needed (and coupled to the explicit check of lenght equality between colors and positions)?

I quickly tested the following fix:

From 880208469cc894285e2f47b5768e4617281c8266 Mon Sep 17 00:00:00 2001
From: Adrien F Vincent <vincent.adrien@gmail.com>
Date: Sat, 4 Mar 2017 11:08:24 +0100
Subject: [PATCH] naive fix for issue 8193

---
 lib/matplotlib/axes/_axes.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/matplotlib/axes/_axes.py b/lib/matplotlib/axes/_axes.py
index 5ffd8a7..e4a617b 100644
--- a/lib/matplotlib/axes/_axes.py
+++ b/lib/matplotlib/axes/_axes.py
@@ -1193,6 +1193,15 @@ or tuple of floats
             lineoffsets = [None]
         if len(colors) == 0:
             colors = [None]
+        try:
+            # Early conversion of the colors into RGBA values to take care
+            # of cases like colors='0.5' or colors='C1'.  (Issue #8193)
+            colors = mcolors.to_rgba_array(colors)
+        except ValueError:
+            # Will fail if any element of *colors* is None. But as long
+            # as len(colors) == 1 or len(positions), the rest of the
+            # code should process *colors* properly.
+            pass
 
         if len(lineoffsets) == 1 and len(positions) != 1:
             lineoffsets = np.tile(lineoffsets, len(positions))
-- 
2.10.2

It seems to do the job. For example, the following script

import matplotlib.pyplot as plt

fig, (ax0, ax1, ax2) = plt.subplots(ncols=3, figsize=(9.6, 4.8))

events = ((1, 4), (2, 5), (3, 6))

colors_A = "C2"  # a realistic case
colors_B = "rkb"  # kind of a corner case
colors_C = ["tab:red", None, "BlAcK"]  # another weird case

for ax, colors in [(ax0, colors_A), (ax1, colors_B), (ax2, colors_C)]:

    ax.eventplot(events, colors=colors)
    ax.set_title("colors={c}".format(c=colors))

    # Cosmeticks
    ax.set_xticks([])
    ax.set_yticks([])

plt.show()

produces
check_naive_fix

And it does not seem to break any of the *eventplot tests I was able to found in test_axes

@anntzer
Copy link
Contributor

anntzer commented Mar 4, 2017

broadcast_to does not require an explicit check (it also works if the sizes already match), avoids a copy (it creates a view), avoids accidentally multiplying an array (instead of a list), and directly gives you an error message if the shapes are non-broadcastable, rather than having to write one yourself again and again:

In [2]: np.broadcast_to([1, 2], 3)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-21ec3fa5ff8b> in <module>()
----> 1 np.broadcast_to([1, 2], 3)

/usr/lib/python3.6/site-packages/numpy/lib/stride_tricks.py in broadcast_to(array, shape, subok)
    172            [1, 2, 3]])
    173     """
--> 174     return _broadcast_to(array, shape, subok=subok, readonly=True)
    175 
    176 

/usr/lib/python3.6/site-packages/numpy/lib/stride_tricks.py in _broadcast_to(array, shape, subok, readonly)
    127     broadcast = np.nditer(
    128         (array,), flags=['multi_index', 'refs_ok', 'zerosize_ok'] + extras,
--> 129         op_flags=[op_flag], itershape=shape, order='C').itviews[0]
    130     result = _maybe_view_as_subclass(array, broadcast)
    131     if needs_writeable and not result.flags.writeable:

ValueError: operands could not be broadcast together with remapped shapes [original->remapped]: (2,) and requested shape (3,)

@afvincent
Copy link
Contributor

For sure, it seems appealing ^^. However, I am not sure that it would exactly fit into the scope of a PR aimed at fixing the current issue: from what I understand, more or less all the parameters of eventplot might benefit from using broadcast_to. May be worth a PR with this specific overhaul?

At the moment, I have a PR on its way, just for the current issue, based on the “naive” fix. I am planning to add some kind of test and to perform some (at least minimal) overhaul of the relevant docstrings (the ones for eventplot and EventCollection seem to be slightly inconsistent).

@anntzer
Copy link
Contributor

anntzer commented Mar 5, 2017

Do you want to review #7562? :-) (which does not fix eventplot, though, I believe).

@afvincent
Copy link
Contributor

Wait, #7562 is so huge! But I'll see what I can do (at least for the parts I can understand) ;).

@anntzer
Copy link
Contributor

anntzer commented Oct 16, 2017

fixed by #8861.

@anntzer anntzer closed this as completed Oct 16, 2017
@QuLogic QuLogic added this to the 2.1 (next point release) milestone Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants