Colours like 'XeYYYY' don't get recognised properly if X, Y's are numbers

[myyc]

'8e1245' gets recognised as 8*10^(1245). While this is intended behaviour, all around matplotlib there seems to be some ambiguity as to whether hex colours should have a trailing hash (e.g.: hex2color fails without hash while colours in a cycler in a stylesheet fail with a hash).

This patch doesn't fix cases such as 0e1123 because they get interpreted as 0.0 so it probably needs a more fundamental decision.

Thoughts?

[myyc]

For the record, this bug arose with a stylesheet having this line

axes.prop_cycle: cycler('color', ['ff496c', '1dacd6', 'b2ec5d', 'e6a8d7', '8e4585', '1cac78', '77dde7', 'ffae42', 'fd7c6e'])

With the fifth colour being interpreted as (inf, inf, inf). Even trying a representation (e.g. using ipython) of said cycler this is what you'd get:

"cycler('color', ['#ff496c', '#1dacd6', '#b2ec5d', '#e6a8d7', '8e4586', '#1cac78', '#77dde7', '#ffae42', '#fd7c6e'])"

Notice the lack of # in the fifth colour, 8e4586.

[tacaswell]

This does not seem like the right fix for the bug you are reporting.

The code that parses rcparams / stylesheets has no way of escaping #. Internally we should be including the hash, but in the style sheets you must not include the hash.

The bug that should be fixed here is in the validation of rcparams. In rcsetup.validate_color it should treat 6 or 8 character strings as hex first, then fall back to treating it as gray scale.

This seems to be fixing a different bug, in that to_rgba will output in-valid colors for some inputs. Can you also add a check that 0 <= c <=1 and some tests that it raises?

attn @anntzer

[myyc]

rcsetup.validate_color calls colors._to_rgba_no_colorcycle if all else fails, so this call will return True before adding the #.

Remember that 0 <= c <=1 is not enough as float('0e0123') == 0.

I am fully aware that my patch doesn't fix the bug itself. One possible way could be to prioritise is_color_like('#' + s) over is_color_like(s) if s is a 6-8char string. This fixes the rc validation, but the overall issue stays.

[tacaswell]

Do not need this block as it will correctly fall through to L380

[tacaswell]

Can you do this with a regex?

[tacaswell]

One possible way could be to prioritise is_color_like('#' + s) over is_color_like(s) if s is a 6-8char string. This fixes the rc validation, but the overall issue stays.

Yes, that is what I was suggesting as the proper fix. The only place a string without a # should be treated as RGB(A) is when parsing the rcparams.

The commit you just pushed looks good. Can you please add a test?

[myyc]

OK as it happens often I'm changing my mind. I committed another fix that does what I said above. The bug as per my first comment seems fixed. hex2color still returns two completely different outcomes for '0e0300' and '0ea300', which is probably a bit dirty, but at least it's not a major issue as people should add '#' to their hex colour codes in their scripts.

[WeatherGod]

Uhm, have we always made it a requirement for hex colors to have the hash?
I am not certain that is true, and requiring it now would likely break
existing code.

On Thu, Jul 7, 2016 at 9:41 AM, myyc notifications@github.com wrote:

OK as it happens often I'm changing my mind. I committed another fix that
does what I said above. The bug as per my first comment seems fixed.
hex2color still returns two completely different outcomes for '0e0300'
and '0ea300', which is probably a bit dirty, but at least it's not a
major issue as people should add '#' to their hex colour codes in their
scripts.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6701 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARy-DfLTqM7ES3GOZsaJM0RoLTdithcks5qTQICgaJpZM4JHA3X
.

[myyc]

I've appended a test to test_colors:test_cn which kept failing because for some reason from cycler import cycler's cycler has the old behaviour. from matplotlib import cycler works fine. Not sure why.

[WeatherGod]

There are two cyclers... there is the cycler from the cycler package, and then there is our cycler that performs validation. The unit tests here depend on the original cyclers.

[myyc]

It's used only in test_cn though. It's no effort reverting this and importing mpl's cycler locally but the only other two instances are in the same method.

[WeatherGod]

Well, what are you trying to test? Both are perfectly legitimate inputs in
different contexts. The mpl cycler is what is used internally when
processing the rc file, but the plain old cycler is probably what gets used
when a user creates their own cycler within a script. The former will
normalize inputs and values as best as it can, while the latter will simply
normalize the key names, and simply validate the values.

On Thu, Jul 7, 2016 at 11:04 AM, myyc notifications@github.com wrote:

It's used only in test_cn though. It's no effort reverting this and
importing mpl's cycler locally but the only other two instances are in the
same method.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6701 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARy-KFncLws3RiojKeSAWtcZDAvko-1ks5qTRWTgaJpZM4JHA3X
.

[anntzer]

Can you add a comment reminding future readers why this needs to happen first?

[anntzer]

I haven't fully followed the discussion about the need for a prefix # but I think the proper fix may be instead to reject scientific notation strings in _to_rgba_no_colorcycle, i.e. replace

try:
    return (float(c),) * 3 + (alpha if alpha is not None else 1.,)
except ValueError:
    pass

by

if <proper regex>: return (float(c),) * 3 + (alpha if alpha is not None else 1.,)

where <proper regex> allows exactly 0, 0.<digits>, 1, 1.0* (left as an exercise to the reader).

[myyc]

You guys should probably decide what you think is best as there are mixed opinions :)

[anntzer]

Another reason to put the fix in the color converter (as I suggested) rather than in rcsetup is that this issue is not limited, well, to the rcsetup.
For example, currently, plot([1, 2], c="1e2345") results in a black line; I'd argue that it should instead raise an error (just like any invalid color) -- only c="0.<digits>" (+ edge cases of 0 and 1) should work as "grays".

[efiring]

I agree with @anntzer.

[tacaswell]

I do not think we currently support hex codes without leading # -> rgb values so I do not see the need to restrict scientific notation going in for the float gray.

However, I think the original commit on this PR (which has been force-pushed away) which raised if the float values for grayscale were outside of [0, 1] should also go in.

---

In [1]: import matplotlib

In [2]: matplotlib.__version__
Out[2]: '2.0.0b1.post117+gb2b37fb'

In [3]: import matplotlib.colors as mc

In [4]: mc.to_rgb
Out[4]: <function matplotlib.colors.to_rgb>

In [5]: mc.to_rgb('aaaaaa')
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
/home/tcaswell/source/p/matplotlib/lib/matplotlib/colors.py in to_rgba(c, alpha)
    132     try:
--> 133         rgba = _colors_full_map.cache[c, alpha]
    134     except (KeyError, TypeError):  # Not in cache, or unhashable.

KeyError: ('aaaaaa', None)

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
<ipython-input-5-bbb3c951fab9> in <module>()
----> 1 mc.to_rgb('aaaaaa')

/home/tcaswell/source/p/matplotlib/lib/matplotlib/colors.py in to_rgb(c)
    233     """Convert `c` to an RGB color, silently dropping the alpha channel.
    234     """
--> 235     return to_rgba(c)[:3]
    236 
    237 

/home/tcaswell/source/p/matplotlib/lib/matplotlib/colors.py in to_rgba(c, alpha)
    133         rgba = _colors_full_map.cache[c, alpha]
    134     except (KeyError, TypeError):  # Not in cache, or unhashable.
--> 135         rgba = _to_rgba_no_colorcycle(c, alpha)
    136         try:
    137             _colors_full_map.cache[c, alpha] = rgba

/home/tcaswell/source/p/matplotlib/lib/matplotlib/colors.py in _to_rgba_no_colorcycle(c, alpha)
    176         except ValueError:
    177             pass
--> 178         raise ValueError("Invalid RGBA argument: {!r}".format(orig_c))
    179     # tuple color.
    180     # Python 2.7 / numpy 1.6 apparently require this to return builtin floats,

ValueError: Invalid RGBA argument: 'aaaaaa'

In [6]: mc.to_rgb('#aaaaaa')
Out[6]: (0.6666666666666666, 0.6666666666666666, 0.6666666666666666)

In [1]: import matplotlib.colors as mc

In [2]: mc.colorConverter
Out[2]: <matplotlib.colors.ColorConverter at 0x7fa58d67f400>

In [3]: mc.colorConverter.to_rgb('000000')
Out[3]: (0.0, 0.0, 0.0)

In [4]: mc.colorConverter.to_rgb('aaaaaa')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
/home/tcaswell/source/p/matplotlib/lib/matplotlib/colors.py in to_rgb(self, arg)
    305                     else:
--> 306                         fl = float(argl)
    307                         if fl < 0 or fl > 1:

ValueError: could not convert string to float: 'aaaaaa'

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
<ipython-input-4-e922cd98d828> in <module>()
----> 1 mc.colorConverter.to_rgb('aaaaaa')

/home/tcaswell/source/p/matplotlib/lib/matplotlib/colors.py in to_rgb(self, arg)
    326         except (KeyError, ValueError, TypeError) as exc:
    327             raise ValueError(
--> 328                 'to_rgb: Invalid rgb arg "%s"\n%s' % (str(arg), exc))
    329             # Error messages could be improved by handling TypeError
    330             # separately; but this should be rare and not too hard

ValueError: to_rgb: Invalid rgb arg "aaaaaa"
could not convert string to float: 'aaaaaa'

In [6]: import matplotlib

In [7]: matplotlib.__version__
Out[7]: '1.5.2'

[anntzer]

I think if someone passes 1e0000 as the c kwarg (or via the figure options UI), there's >95% chance that they really meant #1e0000. Basically, I think the only case where it may make sense to use scientific notation when passing a string gray is for very small values (such as "9e-3"), but even then I would doubt that there's any real world usage of this...

[WeatherGod]

How about we warn if we come across a string that could be miscontrued as something like that? Possibly leading to a deprecation of scientific notation?

[tacaswell]

This line needs to be wrapped to 80 chr

[tacaswell]

I propose the following:

• fix the pep8 in this PR and get it merged
• relax the parsing of hex colors / restrict the string gray colors in a separate PR

[anntzer]

Just to be clear, I think hex colors without a leading # should still not be allowed (in fact, if we could break backwards compatibility I'd make sure the # is enforced consistently everywhere), so c="1e0000" should just become an error.

[tacaswell]

Great, sorry I misread.

I am not where where, other than in the rcparams files (which have parsing issues), we accept hex with out a leading #.

[anntzer]

Ah, of course, I didn't realize there was the issue with parsing comments... forget what I said 😃

[tacaswell]

👍 Thanks @myyc

[tacaswell]

Backported to v2.x as 3f72974