New axes.set_prop_cycle method cannot handle any generic iterable

[u55]

Dear matplotlib developers,

The docstring for the new method axes.set_prop_cycle, says that it can take any iterable for the property value. However, I found that it fails with a ValueError if given a numpy.ndarray. Here is a minimal working example:

from __future__ import print_function
import numpy as np
import matplotlib.pyplot as plt
from matplotlib import cm

cmap = cm.Set1(np.linspace(0,1,9))
print(type(cmap),'\n')

fig, ax = plt.subplots()
ax.set_prop_cycle('color', cmap.tolist())    # works
ax.set_prop_cycle('color', cmap)    # raises ValueError
plt.show()

which produces this traceback:

<type 'numpy.ndarray'> 

Traceback (most recent call last):
  File "/tmp/untitled.py", line 12, in <module>
    ax.set_prop_cycle('color', cmap)    # raises ValueError
  File "/usr/lib/python2.7/site-packages/matplotlib/axes/_base.py", line 1112, in set_prop_cycle
    prop_cycle = cycler(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/matplotlib/rcsetup.py", line 720, in cycler
    vals = validator(vals)
  File "/usr/lib/python2.7/site-packages/matplotlib/rcsetup.py", line 90, in f
    raise ValueError(msg)
ValueError: 's' must be of type [ string | list | tuple ]

Tested with matplotlib 1.5.0, python 2.7 and python 3.5, running in Arch Linux.
This failure in matplotlib 1.5.0 also affects the method axes.set_color_cycle, which is supposed to be deprecated but raises a ValueError instead. The following code worked in matplotlib 1.4.3 but fails in 1.5.0:

import numpy as np
import matplotlib.pyplot as plt
from matplotlib import cm

cmap = cm.Set1(np.linspace(0,1,9))

fig, ax = plt.subplots()
ax.set_color_cycle(cmap)    # raises ValueError
plt.show()

I think that, for convenience and for backwards compatibility with prior versions of axes.set_color_cycle, the new method axes.set_prop_cycle should be able to take any iterable, as the documentation suggests, and not be limited to only a string, list, or tuple.

Thank you,

[tacaswell]

Can you take a look at how this validation is done? The intention is definitely to take any iterable.

Attn @WeatherGod

[u55]

Yes, the validation comes about because axes/_base.py has the line:

from matplotlib.rcsetup import cycler

instead of using something such as:

from cycler import cycler

and the version of cycler in rcsetup.py does the validating, including this test:

elif type(s) in (list, tuple):

Doing this sort of validating for the input parameters of axes.set_prop_cycle is unpythonic, in my opinion, because it goes against the principle of duck typing.

However, fixing the problem is not a simple matter of replacing from matplotlib.rcsetup import cycler with from cycler import cycler, because axes.set_prop_cycle has three different input forms:

Form 1 simply sets given `Cycler` object.
Form 2 creates and sets  a `Cycler` from a label and an iterable.
Form 3 composes and sets  a `Cycler` as an inner product of the
       pairs of keyword arguments. In other words, all of the
       iterables are cycled simultaneously, as if through zip().

and the logic to handle this is currently in rcsetup.cycler rather than in axes.set_prop_cycle.

[tacaswell]

That test should probably be replaced with a try/except block once the
strings are excluded.

Can you take a crack at that?

On Sat, Oct 31, 2015, 16:51 u55 notifications@github.com wrote:

Yes, the validation comes about because axes/_base.py has the line

matplotlib/lib/matplotlib/axes/_base.py

Line 36 in 38782e3

from matplotlib.rcsetup import cycler

:

from matplotlib.rcsetup import cycler

instead of using something such as:

from cycler import cycler

and the version of cycler in rcsetup.py does the validating, including this
test

matplotlib/lib/matplotlib/rcsetup.py

Line 81 in 38782e3

elif type(s) in (list, tuple):

:

elif type(s) in (list, tuple):

Doing this sort of validating for the input parameters of
axes.set_prop_cycle is unpythonic, in my opinion, because it goes against
the principle of duck typing.

However, fixing the problem is not a simple matter of replacing from
matplotlib.rcsetup import cycler with from cycler import cycler, because
axes.set_prop_cycle has three different input forms

matplotlib/lib/matplotlib/axes/_base.py

Lines 1076 to 1082 in 38782e3

	Form 1 simply sets given `Cycler` object.
	Form 2 creates and sets a `Cycler` from a label and an iterable.
	Form 3 composes and sets a `Cycler` as an inner product of the
	pairs of keyword arguments. In other words, all of the
	iterables are cycled simultaneously, as if through zip().

:

Form 1 simply sets given Cycler object.
Form 2 creates and sets a Cycler from a label and an iterable.
Form 3 composes and sets a Cycler as an inner product of the
pairs of keyword arguments. In other words, all of the
iterables are cycled simultaneously, as if through zip().

and the logic

matplotlib/lib/matplotlib/rcsetup.py

Lines 688 to 712 in 38782e3

	if args and kwargs:
	raise TypeError("cycler() can only accept positional OR keyword "
	"arguments -- not both.")
	elif not args and not kwargs:
	raise TypeError("cycler() must have positional OR keyword arguments")
	if len(args) == 1:
	if not isinstance(args[0], Cycler):
	raise TypeError("If only one positional argument given, it must "
	" be a Cycler instance.")
	c = args[0]
	unknowns = c.keys - (set(_prop_validators.keys()) |
	set(_prop_aliases.keys()))
	if unknowns:
	# This is about as much validation I can do
	raise TypeError("Unknown artist properties: %s" % unknowns)
	else:
	return Cycler(c)
	elif len(args) == 2:
	pairs = [(args[0], args[1])]
	elif len(args) > 2:
	raise TypeError("No more than 2 positional arguments allowed")
	else:
	pairs = six.iteritems(kwargs)

to handle this is currently in rcsetup.cycler rather than in
axes.set_prop_cycle.

—
Reply to this email directly or view it on GitHub
#5368 (comment)
.

[WeatherGod]

The fix is easy. Use cbook.isiterable instead of checking the types. That
check has always been there, but previously it was only applied to values
going through the rcparams route. When I rejiggered everything, I purposely
wanted validation everywhere and just used the available mechanism. I
didn't think about users supplying a ndarray of rgb values because I
focused on the rcparams codepath.
On Oct 31, 2015 4:57 PM, "Thomas A Caswell" notifications@github.com
wrote:

That test should probably be replaced with a try/except block once the
strings are excluded.

Can you take a crack at that?

On Sat, Oct 31, 2015, 16:51 u55 notifications@github.com wrote:

Yes, the validation comes about because axes/_base.py has the line
<

matplotlib/lib/matplotlib/axes/_base.py

Line 36 in 38782e3

from matplotlib.rcsetup import cycler

:

from matplotlib.rcsetup import cycler

instead of using something such as:

from cycler import cycler

and the version of cycler in rcsetup.py does the validating, including
this
test
<

matplotlib/lib/matplotlib/rcsetup.py

Line 81 in 38782e3

elif type(s) in (list, tuple):

:

elif type(s) in (list, tuple):

Doing this sort of validating for the input parameters of
axes.set_prop_cycle is unpythonic, in my opinion, because it goes against
the principle of duck typing.

However, fixing the problem is not a simple matter of replacing from
matplotlib.rcsetup import cycler with from cycler import cycler, because
axes.set_prop_cycle has three different input forms
<

matplotlib/lib/matplotlib/axes/_base.py

Lines 1076 to 1082 in 38782e3

	Form 1 simply sets given `Cycler` object.
	Form 2 creates and sets a `Cycler` from a label and an iterable.
	Form 3 composes and sets a `Cycler` as an inner product of the
	pairs of keyword arguments. In other words, all of the
	iterables are cycled simultaneously, as if through zip().

:

Form 1 simply sets given Cycler object.
Form 2 creates and sets a Cycler from a label and an iterable.
Form 3 composes and sets a Cycler as an inner product of the
pairs of keyword arguments. In other words, all of the
iterables are cycled simultaneously, as if through zip().

and the logic
<

matplotlib/lib/matplotlib/rcsetup.py

Lines 688 to 712 in 38782e3

	if args and kwargs:
	raise TypeError("cycler() can only accept positional OR keyword "
	"arguments -- not both.")
	elif not args and not kwargs:
	raise TypeError("cycler() must have positional OR keyword arguments")
	if len(args) == 1:
	if not isinstance(args[0], Cycler):
	raise TypeError("If only one positional argument given, it must "
	" be a Cycler instance.")
	c = args[0]
	unknowns = c.keys - (set(_prop_validators.keys()) |
	set(_prop_aliases.keys()))
	if unknowns:
	# This is about as much validation I can do
	raise TypeError("Unknown artist properties: %s" % unknowns)
	else:
	return Cycler(c)
	elif len(args) == 2:
	pairs = [(args[0], args[1])]
	elif len(args) > 2:
	raise TypeError("No more than 2 positional arguments allowed")
	else:
	pairs = six.iteritems(kwargs)

to handle this is currently in rcsetup.cycler rather than in
axes.set_prop_cycle.

—
Reply to this email directly or view it on GitHub
<
#5368 (comment)

.

—
Reply to this email directly or view it on GitHub
#5368 (comment)
.

[u55]

@tacaswell @WeatherGod
Question, how should rcsetup._listify_validator behave when passed an empty dictionary?

I tried replacing this line:

elif type(s) in (list, tuple):
    ...

with this line:

import matplotlib.cbook as cbook
elif cbook.iterable(s):
    ...

which fixes the bug, but now this test fails:

...
        {'validator': validate_stringlist,
         'success': (('', []),
                     ('a,b', ['a', 'b']),
                     ('aardvark', ['aardvark']),
                     ('aardvark, ', ['aardvark']),
                     ('aardvark, ,', ['aardvark']),
                     (['a', 'b'], ['a', 'b']),
                     (('a', 'b'), ['a', 'b']),
                     ((1, 2), ['1', '2'])),
            'fail': ((dict(), ValueError),  # <------- this test fails
                     (1, ValueError),)
            },
...

Previously, an empty list would have been validated but an empty dictionary would have raised a ValueError. An empty list must be valid since certain rcParams, such as 'animation.ffmpeg_args', default to an empty list. I see no reason that an empty dictionary should raise a ValueError, so should I remove this failing test or should I add an extra test on this line for an empty dictionary?

[WeatherGod]

That test has been in there for a long time, And I think an unordered
iterable is a bit silly. However, that probably doesn't mean that it is
correct to error out on it. The problem is creating a test that would pass
reliably on it. So, perhaps the code should be a bit tighter and test for
ordered iterables? Perhaps ABC's would help?

On Sun, Nov 1, 2015 at 7:33 PM, u55 notifications@github.com wrote:

@tacaswell https://github.com/tacaswell @WeatherGod
https://github.com/WeatherGod
Question, how should rcsetup._listify_validator behave when passed an
empty dictionary?

I tried replacing this line

matplotlib/lib/matplotlib/rcsetup.py

Line 81 in 38782e3

elif type(s) in (list, tuple):

:

elif type(s) in (list, tuple):
...

with this line:

import matplotlib.cbook as cbookelif cbook.iterable(s):
...

which fixes the bug, but now this test fails

matplotlib/lib/matplotlib/tests/test_rcparams.py

Line 284 in 38782e3

'fail': ((dict(), ValueError),

:

...
{'validator': validate_stringlist,
'success': (('', []),
('a,b', ['a', 'b']),
('aardvark', ['aardvark']),
('aardvark, ', ['aardvark']),
('aardvark, ,', ['aardvark']),
(['a', 'b'], ['a', 'b']),
(('a', 'b'), ['a', 'b']),
((1, 2), ['1', '2'])),
'fail': ((dict(), ValueError), # <------- this test fails
(1, ValueError),)
},
...

Previously, an empty list would have been validated but an empty
dictionary would have raised a ValueError. An empty list must be valid
since certain rcParams, such as

matplotlib/lib/matplotlib/rcsetup.py

Line 1165 in 38782e3

'animation.ffmpeg_args': [[], validate_stringlist],

'animation.ffmpeg_args', default to an empty list. I see no reason that
an empty dictionary should raise a ValueError, so should I remove this
failing test or should I add an extra test on this line

matplotlib/lib/matplotlib/rcsetup.py

Line 81 in 38782e3

elif type(s) in (list, tuple):

for an empty dictionary?

—
Reply to this email directly or view it on GitHub
#5368 (comment)
.

[tacaswell]

The dict should fail because this is expecting a sequence of values, not a mapping.

[u55]

Then I'm not sure how to do the test. A numpy.ndarray reports that it is not an instance of collections.Sequence:

>>> import collections
>>> import numpy as np
>>> isinstance(np.arange(9), collections.Sequence)
False

[u55]

@tacaswell @WeatherGod
I see no reason to reject unordered iterables, such as sets, however I can see why dictionaries are not really appropriate.
How about replacing the validation test with this?:

try:
    import collections.abc as abc
except ImportError:
    # python 2
    import collections as abc
...

...
        elif isinstance(s, abc.Iterable) and not isinstance(s, abc.Mapping):
            ...

This will correctly pass a numpy.ndarray and a generic generator.

[WeatherGod]

I am ok with that. I still don't like the idea of allowing sets and bags,
but I see no reason to prevent them, either.

On Wed, Nov 4, 2015 at 11:23 AM, u55 notifications@github.com wrote:

@tacaswell https://github.com/tacaswell @WeatherGod
https://github.com/WeatherGod
I see no reason to reject unordered iterables, such as sets, however I can
see why dictionaries are not really appropriate.
How about replacing the validation test

matplotlib/lib/matplotlib/rcsetup.py

Line 81 in cb3c6f3

elif type(s) in (list, tuple):

with this?:

try:
import collections.abc as abcexcept ImportError:
# python 2
import collections as abc
...

...
elif isinstance(s, abc.Iterable) and not isinstance(s, abc.Mapping):
...

This will correctly pass a numpy.ndarray and a generic generator.

—
Reply to this email directly or view it on GitHub
#5368 (comment)
.

[tacaswell]

In the case of color_cycle an un-ordered iterable does not make sense as there being an order is important to the function.

I think it is a problem that from process-to-process these validators will give different results.

[WeatherGod]

@tacaswell, note that the validator will convert whatever iterable into a
list, so it becomes ordered for the purpose of the cycler. Yes, which order
that will be is arbitrary, but that is for the user to decide if that is an
issue or not for their code.

On Wed, Nov 4, 2015 at 11:29 AM, Thomas A Caswell notifications@github.com
wrote:

In the case of color_cycle an un-ordered iterable does not make sense as
there being an order is important to the function.

I think it is a problem that from process-to-process these validators will
give different results.

—
Reply to this email directly or view it on GitHub
#5368 (comment)
.

[tacaswell]

I am just forseeing lots of very confused bug reports about randomized color cycles.

[WeatherGod]

Do you really think there are a lot of people keeping configuration values
around as sets?

Besides, I think this is a good compromise given that numpy's NDArray is
not a Sequence, so any check for orderability then falls to whitelisting
object types, which we know is doomed as soon as someone tries to pass a
pandas object through.

On Wed, Nov 4, 2015 at 11:47 AM, Thomas A Caswell notifications@github.com
wrote:

I am just forseeing lots of very confused bug reports about randomized
color cycles.

—
Reply to this email directly or view it on GitHub
#5368 (comment)
.

[tacaswell]

Do not underestimate users 😜

It is probably worth letting that go through to keep the code simple, can we at least add a big developer warning in code comments summarizing this conversation?

[WeatherGod]

Sure:

# @WeatherGod: meh, let the users mess up themselves
# @tacaswell: SETS BAD!

;-)

On Wed, Nov 4, 2015 at 11:57 AM, Thomas A Caswell notifications@github.com
wrote:

Do not underestimate users [image: 😜]

It is probably worth letting that go through to keep the code simple, can
we at least add a big developer warning in code comments summarizing this
conversation?

—
Reply to this email directly or view it on GitHub
#5368 (comment)
.

[WeatherGod]

@u55, I hope the banter between @tacaswell and I didn't discourage you from putting together a pull request with your suggested fix.

[u55]

Yeah, no problem. I'm just busy during the week. I'll submit a pull request soon.

[WeatherGod]

Fixed by #5452 and #5459