The proposed fix looks fine to me.

You may want to look at the selection code in #12572 which squashed a similar bug. If you open a PR, please do add atest so this doesn't happen again. Thanks!

@mwaskom yes, new in 3.0.1.

This is 3.0.0

and in 3.0.1

@jklymak Will take a look there.

Thx

Even with the length check this is still problematic for certain iterables. I currently use the color option to color labeled data, where I label the data with a pandas Series indexed by ID numbers:

ids = range(10,18)
x =pd.Series(np.random.uniform(size=8), index=ids)
y =pd.Series(np.random.uniform(size=8), index=ids)
c =pd.Series([1, 1, 1, 1, 1, 0, 0, 0] , index=ids)
plt.scatter(x, y, c)

This plot allows for coloring the plotted points according to their label in v3.0.0. However, now when we check isinstance(c[0], str), c[0] will result in a KeyError because the pandas Series passed into c is indexed 10-17.

I have at lease one case of this that is breaking between 3.0.0 and 3.0.1.

@montebhoover
In your example, does passing plt.scatter(x, y, c.values) get around the KeyError issue?

Yes that works for me - thanks! Does it work for both of our cases if the proposed fix is:

        if (c_none or
                co is not None or
                isinstance(c, str) or
                (isinstance(c, collections.Sequence) and  # change Iterable to Sequence
                    len(c) > 0 and  # proposed fix here
                    isinstance(c[0], str))):

This allows me to still pass in a pandas Series:

>>> isinstance(pd.Series(), collections.Sequence)
False

but would that cover all the relevant cases? I believe a numpy ndarray is also not a Sequence, so that could cause problems if it contains strings.

For my use I need to use the c.values fix for now, but I'm just trying to think of a solution that provides compatibility between 3.0.0 and 3.0.1.

Pandas series are not iterable? What does no.isiterable give for them?

I think the real issue here is isinstance(c[0], str) - it's a fast but less robust python way to check if c is a iterable of strings. If we had explicit types then we could have avoided this but that's for another day.

Anyway, to make this work for lists, numpy arrays and pandas series, we need to avoid indexing into c with [0]. Could try that by doing one of the following, it only tests the first item of an iterable, which is what the original code does. Testing all is more robust but clearly could be slow if c is big.

import itertools as itr

def _is_iterable_str(x):
    # since we also test if x is collections.Iterable
    # take 1st item, head is an iterator
    head = itr.islice(x, 1)
    # test if first item in head is str
    return isinstance(next(head), str)

# in _axes.py.scatter()...
if (c_none or
    co is not None or
    isinstance(c, str) or
    (isinstance(c, collections.Iterable) and 
     # proposed fix 2 lines below
     len(c) > 0 and 
     _is_iterable_str(x))):

@jklymak @anntzer What do you guys think? It might be a bit of an overkill but would work for python lists, numpy array or pandas series, or any iterables.

Why not just next(iter(x)), which seems just as fine as going through islice? Of course there's also the issue of whether it would be problematic to advance a non-rewindable generator (not sure).
Note that there are other places where we use x[0] for similar pruposes so they should get the same fix (hopefully factored out as a helper).

private (prefixed by underscore) in cbook.

Ok, going to be traveling tomorrow but here's some stuff to get us going. I ended up writing a more general helper function to test element type in an Iterable.

This function below is what I plan to add in cbook.__init__.py, which is then called in _axes.scatter() instead of testing c[0].

The function basically tests if all elements are of the expected type by default, but can test only the first N items as well if specified. I ended up going with islice instead of iter for the first N part but happy to improve if there's a better way.

One thing on this - if the iterable being tested is empty, i.e. len(x) == 0, this function currently returns True. Open to discussion - should this be the expected result?

@jklymak Haven't looked into a test case yet but will give that some thoughts.

Let me know what you guys think.

# in cbook.__init__.py
def _is_iterable_type(x, expected_type, first_n: int = None):
    """
    Tests if the type of elements in an iterable is a given type.

    Parameters:
    -----------
    x:
        Iterable to test

    expected_type:
        Expected type, e.g. expected_type=str

    first_n: (int)
        default None, non-zero positive integer. Tests first N elements in
        x. If None then test all elements.

    Returns:
    --------
    result: (boolean)

    Examples:
    ---------
    >>> # test if first element in a is a string
    >>> _is_iterable_type(a, str, 1)

    """
    # At the moment if x is empty, this function returns True
    # if len(x) < 1:
    #     return False

    if first_n is not None:
        if first_n < 1:
            raise ValueError('Parameter first_n must be an integer greater '
                             'than 0.')
        test_range = itertools.islice(x, first_n)
    else:
        test_range = iter(x)

    results = (isinstance(u, expected_type) for u in test_range)

    return all(results)

Sorry for my typo above but does np.is_iterable help?

Less about testing for iterable here, it's caused by c[0] for testing if the elements in c is of type str.

The function basically tests if all elements are of the expected type by default, but can test only the first N items as well if specified.

Would stick to just testing the first one: it seems needlessly costly to iterate a potentially very long collection in pure python code, and it's not as if we had any sensible behavior for mixed types anyways (I think).

While the general version of this sounds good, I am skeptical of adding a new helper function in a patch release.

Wouldn't the simplest solution be to use https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook/__init__.py#L1606 instead of c[0] and add a pd.Series test ?

safe_first_element() feels like an easy fix.

I wrote test cases for the general method proposed as well. But happy to do either.

I noticed that there isn't any dependency on pandas (couldn't find any reference to import pandas in source). If I add a test case for pandas it'd introduce a dependency. Does this have any undesirable implications?

I know that pd.Series does work with next(iter(x)).

We have a pd fixture to enable pandas-related tests but not introduce it as mandatory dependency.

Where can I find an example?

E.g.

see #12664.

IMHO, this is an easy way to hit the same bug.

import matplotlib.pyplot as plt
import numpy as np

n_points = 4
xs = ys = zs = np.full(n_points, np.nan)
colors = list(np.full(2, 'k'))
plt.scatter(xs, ys, zs, c=colors)
plt.show()

@massich I don't quite understand that example. That errors on colors not being the same length as xs, etc. Which seems correct to me.

@jklymak you are completely right. I mess up while doing the MWE. I'll also close #12664

This is the right snipped

import matplotlib.pyplot as plt
import numpy as np
from mpl_toolkits.mplot3d import Axes3D  # noqa: F401 unused import

fig = plt.figure()
ax = fig.add_subplot(111, projection='3d')

n_points = 4
xs = ys = zs = np.full(n_points, np.nan)
colors = list(np.full(n_points, 'k'))
ax.scatter(xs, ys, zs, c=colors)
plt.show()

Pushed a fix using @tacaswell 's suggestion to use cbook.safe_first_element(). Added a test case for pd.Series for cbook.safe_first_element(), thanks @timhoffm for the pointer.

@tacaswell The test case for pd.Series deliberately used an non-zero-starting index.

@montebhoover @massich this should fix both of your issues.

Let me know what you guys think.

Closed by #12673, afaict.