Skip to content
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

Fix using .get_color() and friends in labels handling #10030

Merged
merged 1 commit into from Dec 20, 2017

Conversation

Projects
None yet
5 participants
@skirpichev
Copy link
Contributor

commented Dec 17, 2017

PR Summary

This code was introduced in v2.1.1 and, apparently, wasn't tested, because .get_color() returns array.

Here is examples (raises ValueError):
sympy/sympy#13730 (comment)
or
https://travis-ci.org/diofant/diofant/jobs/316670834

PR Checklist

  • Has Pytest style unit tests
Fix using .get_color() and friends in labels handling
This code was introduced in v2.1.1 and, apparently, wasn't tested, because .get_color() returns array.

Here is an issue example:
sympy/sympy#13730 (comment)

@dstansby dstansby added this to the v2.1.2 milestone Dec 17, 2017

@dstansby

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2017

I don't think this works because (for example) string colors are valid, and 's'.any() doesn't make any sense - I've put an alternative fix in #10031, does that work for you?

@skirpichev

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2017

I don't think this works

It does work for me (e.g. fix plotting tests in the Diofant), but apparently it breaks some matplotlib tests.

I've put an alternative fix in #10031, does that work for you?

Unfortunately, no. Now I see this exception:

=================================== FAILURES ===================================
____________________________ test_matplotlib_intro _____________________________

c = array([[ 0.12156863,  0.46666667,  0.70588235,  1.        ]]), alpha = None

    def to_rgba(c, alpha=None):
        """Convert `c` to an RGBA color.
    
        If `alpha` is not `None`, it forces the alpha value, except if `c` is
        "none" (case-insensitive), which always maps to `(0, 0, 0, 0)`.
        """
        # Special-case nth color syntax because it should not be cached.
        if _is_nth_color(c):
            from matplotlib import rcParams
            prop_cycler = rcParams['axes.prop_cycle']
            colors = prop_cycler.by_key().get('color', ['k'])
            c = colors[int(c[1]) % len(colors)]
        try:
>           rgba = _colors_full_map.cache[c, alpha]
E           TypeError: unhashable type: 'numpy.ndarray'

../../venv/dev/lib/python3.5/site-packages/matplotlib/colors.py:132: TypeError

During handling of the above exception, another exception occurred:

    @pytest.mark.skipif(matplotlib is None, reason="no matplotlib")
    def test_matplotlib_intro():
        """Examples from the 'introduction' notebook."""
        try:
            name = 'test'
            tmp_file = TmpFileManager.tmp_file
    
            p = plot(x)
            p = plot(x*sin(x), x*cos(x))
            p.extend(p)
            p[0].line_color = lambda a: a
            p[1].line_color = 'b'
            p.title = 'Big title'
            p.xlabel = 'the x axis'
            p[1].label = 'straight line'
            p.legend = True
            p.aspect_ratio = (1, 1)
            p.xlim = (-15, 20)
>           p.save(tmp_file('%s_basic_options_and_colors' % name))

diofant/plotting/tests/test_plot.py:80: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
diofant/plotting/plot.py:181: in save
    self._backend.save(path)
diofant/plotting/plot.py:966: in save
    self.process_series()
diofant/plotting/plot.py:945: in process_series
    if self.ax.legend():
../../venv/dev/lib/python3.5/site-packages/matplotlib/axes/_axes.py:497: in legend
    **kwargs)
../../venv/dev/lib/python3.5/site-packages/matplotlib/legend.py:1403: in _parse_legend_args
    handles, labels = _get_legend_handles_labels(axs, handlers)
../../venv/dev/lib/python3.5/site-packages/matplotlib/legend.py:1361: in _get_legend_handles_labels
    and not _in_handles(handle, label)):
../../venv/dev/lib/python3.5/site-packages/matplotlib/legend.py:1340: in _in_handles
    if (colors.to_rgba(f_h.get_color()) != colors.to_rgba(h.get_color())):
../../venv/dev/lib/python3.5/site-packages/matplotlib/colors.py:134: in to_rgba
    rgba = _to_rgba_no_colorcycle(c, alpha)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

c = array([[ 0.12156863,  0.46666667,  0.70588235,  1.        ]]), alpha = None

    def _to_rgba_no_colorcycle(c, alpha=None):
        """Convert `c` to an RGBA color, with no support for color-cycle syntax.
    
        If `alpha` is not `None`, it forces the alpha value, except if `c` is
        "none" (case-insensitive), which always maps to `(0, 0, 0, 0)`.
        """
        orig_c = c
        if isinstance(c, six.string_types):
            if c.lower() == "none":
                return (0., 0., 0., 0.)
            # Named color.
            try:
                # This may turn c into a non-string, so we check again below.
                c = _colors_full_map[c.lower()]
            except KeyError:
                pass
        if isinstance(c, six.string_types):
            # hex color with no alpha.
            match = re.match(r"\A#[a-fA-F0-9]{6}\Z", c)
            if match:
                return (tuple(int(n, 16) / 255
                              for n in [c[1:3], c[3:5], c[5:7]])
                        + (alpha if alpha is not None else 1.,))
            # hex color with alpha.
            match = re.match(r"\A#[a-fA-F0-9]{8}\Z", c)
            if match:
                color = [int(n, 16) / 255
                         for n in [c[1:3], c[3:5], c[5:7], c[7:9]]]
                if alpha is not None:
                    color[-1] = alpha
                return tuple(color)
            # string gray.
            try:
                return (float(c),) * 3 + (alpha if alpha is not None else 1.,)
            except ValueError:
                pass
            raise ValueError("Invalid RGBA argument: {!r}".format(orig_c))
        # tuple color.
        c = np.array(c)
        if not np.can_cast(c.dtype, float, "same_kind") or c.ndim != 1:
            # Test the dtype explicitly as `map(float, ...)`, `np.array(...,
            # float)` and `np.array(...).astype(float)` all convert "0.5" to 0.5.
            # Test dimensionality to reject single floats.
>           raise ValueError("Invalid RGBA argument: {!r}".format(orig_c))
E           ValueError: Invalid RGBA argument: array([[ 0.12156863,  0.46666667,  0.70588235,  1.        ]])

../../venv/dev/lib/python3.5/site-packages/matplotlib/colors.py:185: ValueError
=============== 1 failed, 4 passed, 2 xfailed in 133.04 seconds ================

(This test coming from https://github.com/diofant/diofant/blob/master/diofant/plotting/tests/test_plot.py).

@tacaswell

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

@skirpichev Can you provide a minimal test that exercises the failing path?

@skirpichev

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2017

@tacaswell, I'll try to do this.

@dstansby

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2017

I've updated my PR so now it should hopefully work with arrays, give it another try @skirpichev

@jklymak

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2017

It would be nice to see the use case that is breaking.

@skirpichev

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2017

@jklymak

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2017

I saw that comment. Sorry this broke something for you. What is the code that triggers the error?

@skirpichev

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2017

@dstansby. yes - updated solution seems to be working.

@jklymak

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2017

The root of the problem is that you are passing a 2-D array instead of a 1-D array as a color. @dstansby is fixing on our end #10031, but you could also fix on your end by making sure you only pass a 1-D array.

@tacaswell

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

@skirpichev From how you are using plot in that test I assume that is not a plot from Matplotlib. Can you please provide an example using only Matplotlib functions / methods that reproduces your problem?

@jklymak

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

I think the sympy folks are looking into it now.

@jklymak jklymak merged commit 415cd6a into matplotlib:master Dec 20, 2017

4 of 8 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
codecov/project/tests 97.03% (-1.84%) compared to 7a6bab7
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
codecov/project/library 56.91% (target 50%)
Details
lgtm analysis: Python No alert changes
Details

meeseeksdev bot pushed a commit that referenced this pull request Dec 20, 2017

@dstansby

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@jklymak I thought my PR superseeded this one?

@jklymak

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

I was confused too. I think somehow the prs were linked and yours was committed on top of this one versus being separate.

@skirpichev skirpichev deleted the skirpichev:skirpichev-patch-1 branch Dec 20, 2017

@QuLogic

This comment has been minimized.

Copy link
Member

commented Dec 20, 2017

Your commits follow this one; ergo merging it also means this is merged. There is no linkage except the commits.

@jklymak

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

FWIW #10064 will wipe them both out ;-) Reviews welcome....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.