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

Very blunt and crude, but maybe does the job! #7

Merged
merged 1 commit into from
Nov 15, 2018

Conversation

hughjonesd
Copy link
Contributor

This seems to work fine for some basic tests:

dfr <- data.frame(a=rnorm(10), b = rnorm(10), grp = sample(1:3, 10, rep=T))

plot(dfr$a, dfr$b, col = viridis::magma(5), pch = 19)

pal2 <- saturation(viridis::magma, .8)
plot(dfr$a, dfr$b, col = pal2(5), pch = 19)

ggplot(dfr, aes(a,b,color=factor(grp))) + geom_point() + 
      saturation(scale_color_brewer(), 0.15)

Needs opacity fixing and some tests writing.

@jonclayden
Copy link
Owner

Great! I think this is a cleaner approach – thanks. There are some subtleties to resolve: I notice from ?continuous_scale and ?discrete_scale that the palette function behaves differently in each case, so that should be handled properly. I also think creating a ggproto subclass, as you did before, is a better bet than directly replacing the palette function. But I'm happy to merge this and then take it from here, unless you have anything to add...?

@hughjonesd
Copy link
Contributor Author

Fine by me. I guess one question to answer is, do you want to extend this to mix(function, “red”) et cetera?... the only obstacle seems to be deciding what colorspace a function lives in....

@jonclayden
Copy link
Owner

True. Although scales does interpolation in Lab space, I think that the colours it generates are ultimately standard R colours, and so they're in sRGB space by default. For other functions, I think it's reasonable to put the onus on the function author to handle the space of its return values.

As for mixtures and complements, I'll look at the feasibility of supporting functions there, but I might end up leaving it for now, as the internals are a bit different.

@jonclayden jonclayden merged commit 53b8d01 into jonclayden:master Nov 15, 2018
This was referenced Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants