From c6f96f495f5b5dd0a15428bdd3c3b063605d57b5 Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Mon, 11 Mar 2019 19:08:49 -0500 Subject: [PATCH 1/8] better handling of colorscale inputs, fixes #1432 --- R/plotly_build.R | 10 +--- R/utils.R | 36 +++++++++--- tests/testthat/test-plotly-colorscale.R | 78 +++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 16 deletions(-) create mode 100644 tests/testthat/test-plotly-colorscale.R diff --git a/R/plotly_build.R b/R/plotly_build.R index d00df27d9a..daac2d78b5 100644 --- a/R/plotly_build.R +++ b/R/plotly_build.R @@ -373,6 +373,9 @@ plotly_build.plotly <- function(p, registerFrames = TRUE) { p <- verify_guides(p) + # verify colorscale attributes are in a sensible data structure + p <- verify_colorscale(p) + # verify plot attributes are legal according to the plotly.js spec p <- verify_attr_names(p) # box up 'data_array' attributes where appropriate @@ -804,11 +807,6 @@ map_color <- function(traces, stroke = FALSE, title = "", colorway, na.color = " colorObj[c("cmin", "cmax")] <- NULL colorObj[["showscale"]] <- default(TRUE) traces[[i]] <- modify_list(colorObj, traces[[i]]) - traces[[i]]$colorscale <- as_df(traces[[i]]$colorscale) - # sigh, contour colorscale doesn't support alpha - if (grepl("contour", traces[[i]][["type"]])) { - traces[[i]]$colorscale[, 2] <- strip_alpha(traces[[i]]$colorscale[, 2]) - } traces[[i]] <- structure(traces[[i]], class = c("plotly_colorbar", "zcolor")) next } @@ -852,8 +850,6 @@ map_color <- function(traces, stroke = FALSE, title = "", colorway, na.color = " traces[[i]] <- modify_list(list(fillcolor = col), traces[[i]]) } - # make sure the colorscale is going to convert to JSON nicely - traces[[i]]$marker$colorscale <- as_df(traces[[i]]$marker$colorscale) } } diff --git a/R/utils.R b/R/utils.R index 6b573eb95b..207c676241 100644 --- a/R/utils.R +++ b/R/utils.R @@ -130,15 +130,6 @@ colorway <- function(p = NULL) { # TODO: make this more unique? crosstalk_key <- function() ".crossTalkKey" -# modifyList turns elements that are data.frames into lists -# which changes the behavior of toJSON -as_df <- function(x) { - if (is.null(x) || is.matrix(x)) return(x) - if (is.list(x) && !is.data.frame(x)) { - setNames(as.data.frame(x), NULL) - } -} - # arrange data if the vars exist, don't throw error if they don't arrange_safe <- function(data, vars) { vars <- vars[vars %in% names(data)] @@ -658,6 +649,33 @@ verify_mode <- function(p) { p } + +verify_colorscale <- function(p) { + p$x$data <- lapply(p$x$data, function(trace) { + trace$colorscale <- colorscale_json(trace$colorscale) + trace$marker$colorscale <- colorscale_json(trace$marker$colorscale) + trace + }) + p +} + +# Coerce `x` into a data structure that can map to a colorscale attribute. +# Note that colorscales can either be the name of a scale (e.g., 'Rainbow') or +# a 2D array (e.g., [[0, 'rgb(0,0,255)', [1, 'rgb(255,0,0)']]) +colorscale_json <- function(x) { + if (!length(x)) return(x) + if (is.character(x)) return(x) + if (is.matrix(x)) { + if (ncol(x) != 2) stop("A colorscale matrix requires two columns") + x <- as.data.frame(x) + x[, 1] <- as.numeric(x[, 1]) + } + if (is.list(x) && length(x) == 2) { + x <- setNames(as.data.frame(x), NULL) + } + x +} + # if an object (e.g. trace.marker) contains a non-default attribute, it has been user-specified user_specified <- function(obj = NULL) { if (!length(obj)) return(FALSE) diff --git a/tests/testthat/test-plotly-colorscale.R b/tests/testthat/test-plotly-colorscale.R new file mode 100644 index 0000000000..b5b1257c94 --- /dev/null +++ b/tests/testthat/test-plotly-colorscale.R @@ -0,0 +1,78 @@ +context("colorscales") + + +test_that("Can specify marker.colorscale", { + p <- plot_ly( + x = c(-9, -6, -5, -3, -1), + y = c(0, 1, 4, 5, 7), + marker = list( + color = 1:5, + colorscale='Rainbow', + showscale = TRUE + ) + ) + l <- expect_doppelganger_built(p, "marker.colorscale") +}) + +test_that("Can specify contour colorscale", { + p <- plot_ly( + x = c(-9, -6, -5, -3, -1), + y = c(0, 1, 4, 5, 7), + z = matrix(c(10, 10.625, 12.5, 15.625, 20, 5.625, 6.25, 8.125, 11.25, 15.625, 2.5, 3.125, 5, 8.125, 12.5, 0.625, 1.25, 3.125, + 6.25, 10.625, 0, 0.625, 2.5, 5.625, 10), nrow = 5, ncol = 5), + type = "contour", + colorscale = 'Rainbow' + ) + l <- expect_doppelganger_built(p, "contour-colorscale") +}) + +test_that("Can provide a color interpolation function", { + p <- plot_ly(dat, x = 1:10, y = 1:10, color = 1:10, colors = scales::colour_ramp(c("red", "green"))) + l <- expect_doppelganger_built(p, "colorRamp") +}) + +test_that("Can specify contour colorscale", { + + plot_colorscale <- function(colorscale) { + plot_ly( + x = 1:10, + y = 1:10, + marker = list( + color = 1:10, + colorscale = colorScale, + showscale = TRUE + ) + ) + } + + colorScale <- list( + val = seq(0, 1, by = 0.1), + col = pal(seq(0, 1, by = 0.1)) + ) + test_list <- plot_colorscale(colorScale) + test_df <- plot_colorscale(as.data.frame(colorScale)) + test_matrix <- plot_colorscale(as.matrix(as.data.frame(colorScale))) + +}) + + + +test_that("marker.colorscale overrides color", { + plot_ly( + x = c(-9, -6, -5, -3, -1), + y = c(0, 1, 4, 5, 7), + color = 1:5, + marker = list( + #color = 6:10, + colorscale='Rainbow', + showscale = TRUE + ) + ) + l <- expect_doppelganger_built(p, "marker.colorscale") +}) + + +test_that("contour colorscale supports alpha", { + p <- plot_ly(z = volcano, type = "contour", stroke = I("black"), alpha = 0.1) + l <- expect_doppelganger_built(p, "contour-alpha") +}) From 2055abdcf10c9fd73c92579bd07a6f6e44c0edd2 Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Mon, 11 Mar 2019 19:45:33 -0500 Subject: [PATCH 2/8] more careful coercion of bare list of length 2 --- R/utils.R | 18 +++++++++++++++-- tests/testthat/test-plotly-colorscale.R | 27 +++++++++---------------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/R/utils.R b/R/utils.R index 207c676241..d3a9fffc9e 100644 --- a/R/utils.R +++ b/R/utils.R @@ -661,7 +661,7 @@ verify_colorscale <- function(p) { # Coerce `x` into a data structure that can map to a colorscale attribute. # Note that colorscales can either be the name of a scale (e.g., 'Rainbow') or -# a 2D array (e.g., [[0, 'rgb(0,0,255)', [1, 'rgb(255,0,0)']]) +# a 2D array (e.g., [[0, 'rgb(0,0,255)'], [1, 'rgb(255,0,0)']]) colorscale_json <- function(x) { if (!length(x)) return(x) if (is.character(x)) return(x) @@ -671,11 +671,25 @@ colorscale_json <- function(x) { x[, 1] <- as.numeric(x[, 1]) } if (is.list(x) && length(x) == 2) { - x <- setNames(as.data.frame(x), NULL) + # ensure a list like this: list(list(0, 0.5, 1), list("red", "white", "blue")) + # converts to the correct dimensions + if (!is.data.frame(x) && can_be_numeric(x[[1]])) { + x <- data.frame( + val = as.numeric(x[[1]]), + col = as.character(x[[2]]), + stringsAsFactors = FALSE + ) + } + x <- setNames(x, NULL) } x } +can_be_numeric <- function(x) { + x <- suppressWarnings(as.numeric(x)) + sum(is.na(x)) == 0 +} + # if an object (e.g. trace.marker) contains a non-default attribute, it has been user-specified user_specified <- function(obj = NULL) { if (!length(obj)) return(FALSE) diff --git a/tests/testthat/test-plotly-colorscale.R b/tests/testthat/test-plotly-colorscale.R index b5b1257c94..dde7d86bfd 100644 --- a/tests/testthat/test-plotly-colorscale.R +++ b/tests/testthat/test-plotly-colorscale.R @@ -39,7 +39,7 @@ test_that("Can specify contour colorscale", { y = 1:10, marker = list( color = 1:10, - colorscale = colorScale, + colorscale = colorscale, showscale = TRUE ) ) @@ -53,22 +53,15 @@ test_that("Can specify contour colorscale", { test_df <- plot_colorscale(as.data.frame(colorScale)) test_matrix <- plot_colorscale(as.matrix(as.data.frame(colorScale))) -}) - - - -test_that("marker.colorscale overrides color", { - plot_ly( - x = c(-9, -6, -5, -3, -1), - y = c(0, 1, 4, 5, 7), - color = 1:5, - marker = list( - #color = 6:10, - colorscale='Rainbow', - showscale = TRUE - ) - ) - l <- expect_doppelganger_built(p, "marker.colorscale") + expect_doppelganger_built(test_list, "test_list") + expect_doppelganger_built(test_df, "test_df") + expect_doppelganger_built(test_matrix, "test_matrix") + + test_list_2 <- plot_colorscale(list(list(0, "rgb(0,0,255)"), list(1, "rgb(0,255,0)"))) + test_list_3 <- plot_colorscale(list(list(0, 1), list("rgb(0,0,255)", "rgb(0,255,0)"))) + + expect_doppelganger_built(test_list_2, "test_list_2") + expect_doppelganger_built(test_list_3, "test_list_3") }) From a72d016c28066a7e16c38cd3b07e125c591e8ead Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Tue, 12 Mar 2019 10:57:15 -0500 Subject: [PATCH 3/8] fix test --- tests/testthat/test-plotly-colorscale.R | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/test-plotly-colorscale.R b/tests/testthat/test-plotly-colorscale.R index dde7d86bfd..6ca6d88030 100644 --- a/tests/testthat/test-plotly-colorscale.R +++ b/tests/testthat/test-plotly-colorscale.R @@ -45,6 +45,7 @@ test_that("Can specify contour colorscale", { ) } + pal <- scales::colour_ramp(c("red", "green")) colorScale <- list( val = seq(0, 1, by = 0.1), col = pal(seq(0, 1, by = 0.1)) From 6ec50ff18b995fde7b7d0e1af552b87cbea3373f Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Tue, 12 Mar 2019 11:16:14 -0500 Subject: [PATCH 4/8] another fix test --- tests/testthat/test-plotly-colorscale.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-plotly-colorscale.R b/tests/testthat/test-plotly-colorscale.R index 6ca6d88030..4999abf588 100644 --- a/tests/testthat/test-plotly-colorscale.R +++ b/tests/testthat/test-plotly-colorscale.R @@ -27,7 +27,7 @@ test_that("Can specify contour colorscale", { }) test_that("Can provide a color interpolation function", { - p <- plot_ly(dat, x = 1:10, y = 1:10, color = 1:10, colors = scales::colour_ramp(c("red", "green"))) + p <- plot_ly(x = 1:10, y = 1:10, color = 1:10, colors = scales::colour_ramp(c("red", "green"))) l <- expect_doppelganger_built(p, "colorRamp") }) From 871ae814ab2424a83ffd26e416f25428bf6167bc Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Tue, 12 Mar 2019 18:15:11 -0500 Subject: [PATCH 5/8] check length of list elements and better check for numeric coercion --- R/utils.R | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/R/utils.R b/R/utils.R index d3a9fffc9e..87b72b5a71 100644 --- a/R/utils.R +++ b/R/utils.R @@ -670,24 +670,28 @@ colorscale_json <- function(x) { x <- as.data.frame(x) x[, 1] <- as.numeric(x[, 1]) } + # ensure a list like this: list(list(0, 0.5, 1), list("red", "white", "blue")) + # converts to the correct dimensions: [[0, 'red'], [0.5, 'white'], [1, 'blue']] if (is.list(x) && length(x) == 2) { - # ensure a list like this: list(list(0, 0.5, 1), list("red", "white", "blue")) - # converts to the correct dimensions - if (!is.data.frame(x) && can_be_numeric(x[[1]])) { + n1 <- length(x[[1]]) + n2 <- length(x[[2]]) + if (n1 != n2 || n1+n2 == 0) { + warning("A colorscale list must of elements of the same (non-zero) length") + } else if (!is.data.frame(x) && can_be_numeric(x[[1]])) { x <- data.frame( val = as.numeric(x[[1]]), col = as.character(x[[2]]), stringsAsFactors = FALSE ) + x <- setNames(x, NULL) } - x <- setNames(x, NULL) } x } can_be_numeric <- function(x) { - x <- suppressWarnings(as.numeric(x)) - sum(is.na(x)) == 0 + xnum <- suppressWarnings(as.numeric(x)) + sum(is.na(x)) == sum(is.na(xnum)) } # if an object (e.g. trace.marker) contains a non-default attribute, it has been user-specified From 9a99a48f44377c8ab9b598a57571f9d3d39dc76e Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Wed, 13 Mar 2019 09:49:00 -0500 Subject: [PATCH 6/8] add missing value in test to marker.color --- tests/testthat/test-plotly-colorscale.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-plotly-colorscale.R b/tests/testthat/test-plotly-colorscale.R index 4999abf588..9f328a9752 100644 --- a/tests/testthat/test-plotly-colorscale.R +++ b/tests/testthat/test-plotly-colorscale.R @@ -38,7 +38,7 @@ test_that("Can specify contour colorscale", { x = 1:10, y = 1:10, marker = list( - color = 1:10, + color = c(1:9, NA), colorscale = colorscale, showscale = TRUE ) From 8776482e4431c08ef3a75a701c9aeace5e01d11e Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Wed, 13 Mar 2019 09:55:08 -0500 Subject: [PATCH 7/8] add new baselines --- tests/figs/colorscales/colorramp.svg | 1 + tests/figs/colorscales/contour-alpha.svg | 1 + tests/figs/colorscales/contour-colorscale.svg | 1 + tests/figs/colorscales/marker-colorscale.svg | 1 + tests/figs/colorscales/test-df.svg | 1 + tests/figs/colorscales/test-list-2.svg | 1 + tests/figs/colorscales/test-list-3.svg | 1 + tests/figs/colorscales/test-list.svg | 1 + tests/figs/colorscales/test-matrix.svg | 1 + 9 files changed, 9 insertions(+) create mode 100644 tests/figs/colorscales/colorramp.svg create mode 100644 tests/figs/colorscales/contour-alpha.svg create mode 100644 tests/figs/colorscales/contour-colorscale.svg create mode 100644 tests/figs/colorscales/marker-colorscale.svg create mode 100644 tests/figs/colorscales/test-df.svg create mode 100644 tests/figs/colorscales/test-list-2.svg create mode 100644 tests/figs/colorscales/test-list-3.svg create mode 100644 tests/figs/colorscales/test-list.svg create mode 100644 tests/figs/colorscales/test-matrix.svg diff --git a/tests/figs/colorscales/colorramp.svg b/tests/figs/colorscales/colorramp.svg new file mode 100644 index 0000000000..339a37794b --- /dev/null +++ b/tests/figs/colorscales/colorramp.svg @@ -0,0 +1 @@ +246810246810246810 diff --git a/tests/figs/colorscales/contour-alpha.svg b/tests/figs/colorscales/contour-alpha.svg new file mode 100644 index 0000000000..9845f0dbcb --- /dev/null +++ b/tests/figs/colorscales/contour-alpha.svg @@ -0,0 +1 @@ +010203040506001020304050607080100120140160180 diff --git a/tests/figs/colorscales/contour-colorscale.svg b/tests/figs/colorscales/contour-colorscale.svg new file mode 100644 index 0000000000..8abb4e1f11 --- /dev/null +++ b/tests/figs/colorscales/contour-colorscale.svg @@ -0,0 +1 @@ +−8−6−4−201234567481216 diff --git a/tests/figs/colorscales/marker-colorscale.svg b/tests/figs/colorscales/marker-colorscale.svg new file mode 100644 index 0000000000..27e91b837c --- /dev/null +++ b/tests/figs/colorscales/marker-colorscale.svg @@ -0,0 +1 @@ +−8−6−4−20123456711.522.533.544.55 diff --git a/tests/figs/colorscales/test-df.svg b/tests/figs/colorscales/test-df.svg new file mode 100644 index 0000000000..e66b88ad0c --- /dev/null +++ b/tests/figs/colorscales/test-df.svg @@ -0,0 +1 @@ +246810246810123456789 diff --git a/tests/figs/colorscales/test-list-2.svg b/tests/figs/colorscales/test-list-2.svg new file mode 100644 index 0000000000..95b381bef2 --- /dev/null +++ b/tests/figs/colorscales/test-list-2.svg @@ -0,0 +1 @@ +246810246810123456789 diff --git a/tests/figs/colorscales/test-list-3.svg b/tests/figs/colorscales/test-list-3.svg new file mode 100644 index 0000000000..95b381bef2 --- /dev/null +++ b/tests/figs/colorscales/test-list-3.svg @@ -0,0 +1 @@ +246810246810123456789 diff --git a/tests/figs/colorscales/test-list.svg b/tests/figs/colorscales/test-list.svg new file mode 100644 index 0000000000..e66b88ad0c --- /dev/null +++ b/tests/figs/colorscales/test-list.svg @@ -0,0 +1 @@ +246810246810123456789 diff --git a/tests/figs/colorscales/test-matrix.svg b/tests/figs/colorscales/test-matrix.svg new file mode 100644 index 0000000000..e66b88ad0c --- /dev/null +++ b/tests/figs/colorscales/test-matrix.svg @@ -0,0 +1 @@ +246810246810123456789 From 0941f05dc6c763d2a0b04322db8e9301b5a678b1 Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Wed, 13 Mar 2019 16:04:32 -0500 Subject: [PATCH 8/8] more transparent logic --- R/utils.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/utils.R b/R/utils.R index 87b72b5a71..8c1496185b 100644 --- a/R/utils.R +++ b/R/utils.R @@ -675,7 +675,7 @@ colorscale_json <- function(x) { if (is.list(x) && length(x) == 2) { n1 <- length(x[[1]]) n2 <- length(x[[2]]) - if (n1 != n2 || n1+n2 == 0) { + if (n1 != n2 || n1 == 0 || n2 == 0) { warning("A colorscale list must of elements of the same (non-zero) length") } else if (!is.data.frame(x) && can_be_numeric(x[[1]])) { x <- data.frame(