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

ColorScaleValidator should probably return a string when it's specified as a string. #1087

Closed
ivirshup opened this issue Aug 1, 2018 · 2 comments
Labels
bug something broken
Milestone

Comments

@ivirshup
Copy link
Contributor

ivirshup commented Aug 1, 2018

Currently, color scale attributes have a kinda weird behavior:

import plotly.graph_objs as go
go.scatter.Marker(colorscale="Viridis").colorscale
(('V',), ('i',), ('r',), ('i',), ('d',), ('i',), ('s',))

This is unlike the behavior (to the best of my knowledge) for other attributes specified as strings:

go.scatter.Marker(color="red").color
'red'

And this is pretty unintuitive behavior when doing stuff like:

marker1 = go.scatter.Marker(colorscale="Viridis")
go.scatter.Marker(colorscale=marker1.colorscale)
ValueError: 
    Invalid value of type 'builtins.tuple' received for the 'colorscale' property of scatter.marker
        Received value: (('V',), ('i',), ('r',), ('i',), ('d',), ('i',), ('s',))

Though .update(marker1) does work for some reason.

My guess is all that needs to happen is to add an elif isinstance(v, str): return v to this function, since you won't be breaking that immutability requirement.

def present(self, v):
# Return tuple of tuples so that colorscale is immutable
if v is None:
return None
else:
return tuple([tuple(e) for e in v])

@jonmmease
Copy link
Contributor

Thanks for the report @ivirshup . Yeah, this is definitely an oversight. You're proposed fix should take care of it. Feel free to submit a PR, otherwise we'll get to in the next few weeks. Thanks!

@jonmmease jonmmease added the bug something broken label Aug 1, 2018
ivirshup added a commit to ivirshup/plotly.py that referenced this issue Aug 2, 2018
Added a test checking that a colorscale specified as a string is returned as a string. Previously it had been returned as a tuple of 1-tuples. e.g. "Viridis" -> (('V',), ('i',), ('r',), ('i',), ('d',), ('i',), ('s',)). Catches plotly#1087.
ivirshup added a commit to ivirshup/plotly.py that referenced this issue Aug 2, 2018
ivirshup added a commit to ivirshup/plotly.py that referenced this issue Aug 3, 2018
Added tests checking that a colorscale specified as a string is returned correctly. Previously it had been returned as a tuple of 1-tuples. e.g. "Viridis" -> (('V',), ('i',), ('r',), ('i',), ('d',), ('i',), ('s',)). Catches plotly#1087.
ivirshup added a commit to ivirshup/plotly.py that referenced this issue Aug 3, 2018
jonmmease pushed a commit that referenced this issue Aug 6, 2018
* Added tests for colorscales specified as a string

Added tests checking that a colorscale specified as a string is returned correctly. Previously it had been returned as a tuple of 1-tuples. e.g. "Viridis" -> (('V',), ('i',), ('r',), ('i',), ('d',), ('i',), ('s',)). Catches #1087.

* Fix presentation of string colorscales

Fixes #1087.

* Added support for Cividis to ColorScaleValidator
@jonmmease jonmmease added this to the v3.1.1 milestone Aug 7, 2018
@jonmmease
Copy link
Contributor

Hi @ivirshup ,

I just pushed out release candidates for plotly.py 3.1.1 and plotlywidget 0.2.1. Installation instructions for the release candidates are at https://github.com/plotly/plotly.py/blob/bc1d4d188ab999cd9c21e3a7908729f156bc200f/README.md.

If you have a chance to confirm that this issue is resolved in the release candidate that would be awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

No branches or pull requests

2 participants