-
Notifications
You must be signed in to change notification settings - Fork 12
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
ENH: scatter plot support for colormap #30
base: master
Are you sure you want to change the base?
Conversation
Looks good. One concern I have is that the names of matplotlib colormaps do not correspond to the names of vega schemes, but there's not really any easy way around that. |
can we for all those params support both strings and |
I'd lean toward no. I think the main benefit of this API is the ability to create charts that basically work on multiple backends. If we start supporting altair-specific extensions of the core API, then that flexibility goes away, and it may not be apparent to users when they have created a chart that will no longer render with one of the other backends. |
@jakevdp how would you like to proceed on this? |
Let's go ahead and apply colormap directly to scheme as here. Not a perfect solution, but I think it's the best thing that can be done easily at this point. First tests and a couple minor changes. |
raise ValueError("'c' should be defined for using 'colormap'") | ||
encodings["color"] = alt.Color( | ||
encodings["color"], scale=alt.Scale(scheme=kwargs.get("colormap")) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this in the if c is not None
block above, so that colormap just has no effect unless c
is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was doing so, to avoid touching columns = list(set(encodings.values()))
. Once this block goes up. list(set(encodings.values()))
would have to be tweaked to extract value > shorthand > field
See: pandas.DataFrame.plot.scatter
If this looks good, I'll update tests.