Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add built-in support for viridis color scales #2178

Merged
merged 15 commits into from Jul 13, 2017

Conversation

Projects
None yet
2 participants
Collaborator

karawoo commented Jun 22, 2017

Related to 1526. This is a first stab at adding built-in support for viridis color scales using the viridisLite package. The function names are kind of long and awkward right now, but I'm not sure what better names would be. Also, should the viridis_pal function go as a PR to scales instead of ggplot2?

karawoo requested a review from hadley Jun 22, 2017

@hadley

Not sure what to do about the names. Maybe just _c and _d?

R/scale-viridis.r
@@ -0,0 +1,68 @@
+#' Viridis colour scales from viridisLite
+#'
+#' @description
@hadley

hadley Jun 22, 2017

Owner

No need to use @description here

R/scale-viridis.r
+#' The `viridis` scales provide color maps that are perceptually uniform in both
+#' color and black-and-white. They are also designed to be perceived by viewers
+#' with common forms of color blindness. See also
+#' \url{https://bids.github.io/colormap/}.
@hadley

hadley Jun 22, 2017

Owner

I think you can use the markdown <https://bids.github.io/colormap/>

R/scale-viridis.r
+#' @export
+scale_colour_viridis_discrete <- function(..., alpha = 1, begin = 0, end = 1,
+ direction = 1, option = "D") {
+ discrete_scale("colour", "viridis_discrete",
@hadley

hadley Jun 22, 2017

Owner

Can you please use tidyverse style here: http://style.tidyverse.org/syntax.html#long-lines

R/scale-viridis.r
+#' @inheritParams viridisLite::viridis
+#' @export
+#' @rdname scale_viridis
+viridis_pal <- function(alpha = 1, begin = 0, end = 1, direction = 1, option= "D") {
@hadley

hadley Jun 22, 2017

Owner

Yes, this should probably go into scales. You'll need to do a PR to scales, including a bump to the version number. Then in this PR, you'll need to adjust the version of scales required in the description, and add Remotes: hadley/scales.

karawoo referenced this pull request in hadley/scales Jun 22, 2017

Merged

Add viridis palette #90

Collaborator

karawoo commented Jun 23, 2017 edited

I need to add some examples to the documentation here. Should I test these scales as well?

Collaborator

karawoo commented Jun 26, 2017

Added a couple tests, one of which very awkwardly (!) checks that scales named "viridis_d" or "viridis_c" have been added. The second test checks that point color changes when adding a viridis scale.

tests/testthat/test-viridis.R
+
+df <- data.frame(x = 1, y = 1, z = "a")
+
+test_that("Viridis color scale gets used", {
@hadley

hadley Jun 27, 2017

Owner

I think you can drop this one

tests/testthat/test-viridis.R
+ geom_point()
+ p2 <- p1 + scale_colour_viridis_d()
+
+ expect_false(layer_data(p1)$colour == layer_data(p2)$colour)
@hadley

hadley Jun 27, 2017

Owner

And can you test the specific colour here?

+#' # The above are equivalent to
+#' v + scale_fill_gradient()
+#' v + scale_fill_viridis_c()
+scale_colour_continuous <- function(...,
@hadley

hadley Jul 3, 2017

Owner

I now think it's probably better form to use the second argument to getOption(), rather than setting on load.

R/scale-viridis.r
+ gradient_n_pal(
+ viridis_pal(alpha, begin, end, direction, option)(6),
+ values,
+ space),
@hadley

hadley Jul 3, 2017

Owner

) should go on new line

NEWS.md
@@ -1,5 +1,7 @@
# ggplot2 2.2.1.9000
+* Add built-in support for `viridis` and related color maps (@karawoo).
@hadley

hadley Jul 3, 2017

Owner

Can you add a brief comment about the option too?

karawoo added some commits Jun 21, 2017

@karawoo karawoo Begin viridis scales 3942e30
@karawoo karawoo Add to NEWS.md e7174ff
@karawoo karawoo Add documentation for continuous_scale parameters na.value and guide 92d92d7
@karawoo karawoo Remove @description d868d8e
@karawoo karawoo tidyverse style for long calls to discrete_scale/continuous_scale
oops
ff59231
@karawoo karawoo Use markdown url syntax 6f55d9f
@karawoo karawoo viridis_pal is now in the scales package 101397b
@karawoo karawoo Shorten names to viridis_c and viridis_d 8bd3b7c
@karawoo karawoo Add viridis examples based on the brewer examples 6920076
@karawoo karawoo Add viridis tests 769c515
@karawoo karawoo Update viridis tests 94582b8
@karawoo karawoo Use global options to control default continuous colour/fill scales eaf3ddf
@karawoo karawoo Don't set options in .onLoad() d76ac85
@karawoo karawoo Put closing paren on new line dc77651
@karawoo karawoo Update NEWS.md
ec3ae12
@hadley

hadley approved these changes Jul 13, 2017

@karawoo karawoo merged commit 77fc5c7 into tidyverse:master Jul 13, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@karawoo karawoo added a commit to karawoo/ggplot2 that referenced this pull request Jul 14, 2017

@karawoo @karawoo karawoo + karawoo Add built-in support for viridis color scales (#2178)
* Begin viridis scales

* Add to NEWS.md

* Add documentation for continuous_scale parameters na.value and guide

* Remove @description

* tidyverse style for long calls to discrete_scale/continuous_scale

oops

* Use markdown url syntax

* viridis_pal is now in the scales package

* Shorten names to viridis_c and viridis_d

* Add viridis examples based on the brewer examples

* Add viridis tests

* Update viridis tests

* Use global options to control default continuous colour/fill scales

* Don't set options in .onLoad()

* Put closing paren on new line

* Update NEWS.md
0a57d75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment