From c40657a0884452836ec0c3fd556f7ac956ed4e82 Mon Sep 17 00:00:00 2001 From: Stefan Moog Date: Sat, 31 Aug 2024 15:33:07 +0200 Subject: [PATCH 1/3] Fix. Add special treatment of `dimensions` attrbute to account for the data_array propery of the "values" attribute. Closes #2385. * Add tests to check that "values" property has class "AsIs" for "parcoords", "parcats" and "splom" traces. * Add NEWS entry --- NEWS.md | 2 ++ R/utils.R | 10 +++++++++- tests/testthat/test-plotly-parcats.R | 20 ++++++++++++++++++++ tests/testthat/test-plotly-parcoords.R | 20 ++++++++++++++++++++ tests/testthat/test-plotly-splom.R | 22 ++++++++++++++++++++-- 5 files changed, 71 insertions(+), 3 deletions(-) create mode 100644 tests/testthat/test-plotly-parcats.R create mode 100644 tests/testthat/test-plotly-parcoords.R diff --git a/NEWS.md b/NEWS.md index fed423fe24..cc6373aeb8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,6 +12,8 @@ * Closed #2337: Creating a new `event_data()` handler no longer causes a spurious reactive update of existing `event_data()`s. (#2339) +* Closed #2385: `"parcoords"`, `"parcats"` and `"splom"` traces now work with one-dimensional data by accounting for the "data_array" property of the "values" attribute of "dimensions". + # 4.10.4 ## Improvements diff --git a/R/utils.R b/R/utils.R index da79d43cf2..3705fc1eb6 100644 --- a/R/utils.R +++ b/R/utils.R @@ -551,7 +551,15 @@ verify_attr <- function(proposed, schema, layoutAttr = FALSE) { # do the same for "sub-attributes" if (identical(role, "object") && is.recursive(proposed[[attr]])) { - proposed[[attr]] <- verify_attr(proposed[[attr]], attrSchema, layoutAttr = layoutAttr) + # The "dimensions" attribute requires a special treatment as + # it is an unnamed list and hence will be skipped by `for (attr in names(proposed)) + if (attr == "dimensions") { + proposed[[attr]] <- lapply(proposed[[attr]], \(x) { + verify_attr(x, attrSchema$items$dimension, layoutAttr = layoutAttr) + }) + } else { + proposed[[attr]] <- verify_attr(proposed[[attr]], attrSchema, layoutAttr = layoutAttr) + } } } diff --git a/tests/testthat/test-plotly-parcats.R b/tests/testthat/test-plotly-parcats.R new file mode 100644 index 0000000000..7477bbfc09 --- /dev/null +++ b/tests/testthat/test-plotly-parcats.R @@ -0,0 +1,20 @@ +expect_traces <- function(p, n.traces, name) { + stopifnot(is.numeric(n.traces)) + L <- expect_doppelganger_built(p, paste0("plotly-", name)) + expect_equivalent(length(L$data), n.traces) + L +} + +test_that("values property has a class of AsIs", { + p <- plot_ly( + dimensions = list( + list(values = "A"), + list(values = "B") + ), + type = "parcats" + ) + l <- expect_traces(p, 1, "parcats-data-array") + tr <- l$data[[1]]$dimensions + expect_true(inherits(tr[[1]]$values, "AsIs")) + expect_true(inherits(tr[[2]]$values, "AsIs")) +}) diff --git a/tests/testthat/test-plotly-parcoords.R b/tests/testthat/test-plotly-parcoords.R new file mode 100644 index 0000000000..180b466903 --- /dev/null +++ b/tests/testthat/test-plotly-parcoords.R @@ -0,0 +1,20 @@ +expect_traces <- function(p, n.traces, name) { + stopifnot(is.numeric(n.traces)) + L <- expect_doppelganger_built(p, paste0("plotly-", name)) + expect_equivalent(length(L$data), n.traces) + L +} + +test_that("values property has a class of AsIs", { + p <- plot_ly( + dimensions = list( + list(label = "A", values = 3), + list(label = "B", values = 8) + ), + type = "parcoords" + ) + l <- expect_traces(p, 1, "parcoords-data-array") + tr <- l$data[[1]]$dimensions + expect_true(inherits(tr[[1]]$values, "AsIs")) + expect_true(inherits(tr[[2]]$values, "AsIs")) +}) diff --git a/tests/testthat/test-plotly-splom.R b/tests/testthat/test-plotly-splom.R index 5580bff1ee..ef13df0aaa 100644 --- a/tests/testthat/test-plotly-splom.R +++ b/tests/testthat/test-plotly-splom.R @@ -1,5 +1,9 @@ - - +expect_traces <- function(p, n.traces, name) { + stopifnot(is.numeric(n.traces)) + L <- expect_doppelganger_built(p, paste0("plotly-", name)) + expect_equivalent(length(L$data), n.traces) + L +} test_that("No cartesian axes are supplied to a splom chart", { @@ -12,3 +16,17 @@ test_that("No cartesian axes are supplied to a splom chart", { ) }) + +test_that("values property has a class of AsIs", { + p <- plot_ly( + type = "splom", + dimensions = list( + list(values = 1, label = "A"), + list(values = 2, label = "B") + ) + ) + l <- expect_traces(p, 1, "parcats-data-array") + tr <- l$data[[1]]$dimensions + expect_true(inherits(tr[[1]]$values, "AsIs")) + expect_true(inherits(tr[[2]]$values, "AsIs")) +}) From 80a3e5e4e6eb705547fe0870849d957d786aba3e Mon Sep 17 00:00:00 2001 From: Stefan Moog Date: Fri, 6 Sep 2024 12:12:13 +0200 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Carson Sievert --- R/utils.R | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/R/utils.R b/R/utils.R index 3705fc1eb6..c8cac21483 100644 --- a/R/utils.R +++ b/R/utils.R @@ -551,14 +551,18 @@ verify_attr <- function(proposed, schema, layoutAttr = FALSE) { # do the same for "sub-attributes" if (identical(role, "object") && is.recursive(proposed[[attr]])) { - # The "dimensions" attribute requires a special treatment as - # it is an unnamed list and hence will be skipped by `for (attr in names(proposed)) - if (attr == "dimensions") { - proposed[[attr]] <- lapply(proposed[[attr]], \(x) { - verify_attr(x, attrSchema$items$dimension, layoutAttr = layoutAttr) - }) + # Some attributes (e.g., dimensions, transforms) are actually + # a list of objects (even though they, confusingly, have role: object) + # In those cases, we actually want to verify each list element + attr_ <- sub("s$", "", attr) + is_list_attr <- ("items" %in% names(attrSchema)) && + (attr_ %in% names(attrSchema$items)) + if (is_list_attr) { + proposed[[attr]] <- lapply(proposed[[attr]], function(x) { + verify_attr(x, attrSchema$items[[attr_]]) + }) } else { - proposed[[attr]] <- verify_attr(proposed[[attr]], attrSchema, layoutAttr = layoutAttr) + proposed[[attr]] <- verify_attr(proposed[[attr]], attrSchema, layoutAttr = layoutAttr) } } } From bb48de83b0cb3bae3866d26018f14e799d7a485f Mon Sep 17 00:00:00 2001 From: Stefan Moog Date: Fri, 6 Sep 2024 12:13:06 +0200 Subject: [PATCH 3/3] Update NEWS.md Co-authored-by: Carson Sievert --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index cc6373aeb8..ddda02d1af 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,7 +12,7 @@ * Closed #2337: Creating a new `event_data()` handler no longer causes a spurious reactive update of existing `event_data()`s. (#2339) -* Closed #2385: `"parcoords"`, `"parcats"` and `"splom"` traces now work with one-dimensional data by accounting for the "data_array" property of the "values" attribute of "dimensions". +* Closed #2385: `"parcoords"`, `"parcats"` and `"splom"` traces now work when `values` is a length 1 vector (also, more generally, attributes that expect a list of objects should have those objects "verified" correctly now). # 4.10.4