From bb07f4d9caba142387e44fb0cc359f07512f6475 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 21 Jun 2024 10:35:06 +0200 Subject: [PATCH 1/4] precompute checks and lengths --- R/scale-hue.R | 13 ++++++++----- tests/testthat/test-scale-hue.R | 3 +-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/R/scale-hue.R b/R/scale-hue.R index ba50f81dc2..9fca16a506 100644 --- a/R/scale-hue.R +++ b/R/scale-hue.R @@ -205,12 +205,15 @@ scale_fill_qualitative <- function(name = waiver(), ..., type = NULL, #' @param type a character vector or a list of character vectors #' @noRd pal_qualitative <- function(type, h, c, l, h.start, direction) { + type_list <- type + if (!is.list(type_list)) { + type_list <- list(type_list) + } + if (!all(vapply(type_list, is.character, logical(1)))) { + stop_input_type(type, "a character vector or list of character vectors") + } + type_lengths <- lengths(type_list) function(n) { - type_list <- if (!is.list(type)) list(type) else type - if (!all(vapply(type_list, is.character, logical(1)))) { - cli::cli_abort("{.arg type} must be a character vector or a list of character vectors.") - } - type_lengths <- lengths(type_list) # If there are more levels than color codes default to pal_hue() if (max(type_lengths) < n) { return(scales::pal_hue(h, c, l, h.start, direction)(n)) diff --git a/tests/testthat/test-scale-hue.R b/tests/testthat/test-scale-hue.R index 12568590a8..6f0b0c5234 100644 --- a/tests/testthat/test-scale-hue.R +++ b/tests/testthat/test-scale-hue.R @@ -1,6 +1,5 @@ test_that("scale_hue() checks the type input", { - pal <- pal_qualitative(type = 1:4) - expect_snapshot_error(pal(4)) + expect_snapshot_error(pal_qualitative(type = 1:4)) pal <- pal_qualitative(type = colors()) expect_silent(pal(4)) pal <- pal_qualitative(type = list(colors()[1:10], colors()[11:30])) From 699d2d022e17ff3f36138b41d5629a0ef48a24b3 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 21 Jun 2024 10:35:38 +0200 Subject: [PATCH 2/4] don't use loop to select minimal palette --- R/scale-hue.R | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/R/scale-hue.R b/R/scale-hue.R index 9fca16a506..951e784c16 100644 --- a/R/scale-hue.R +++ b/R/scale-hue.R @@ -219,11 +219,8 @@ pal_qualitative <- function(type, h, c, l, h.start, direction) { return(scales::pal_hue(h, c, l, h.start, direction)(n)) } # Use the minimum length vector that exceeds the number of levels (n) - type_list <- type_list[order(type_lengths)] - i <- 1 - while (length(type_list[[i]]) < n) { - i <- i + 1 - } - type_list[[i]][seq_len(n)] + i <- which(type_lengths >= n) + i <- i[which.min(type_lengths[i])] + type_list[[i]] } } From 5cb61b96316ff25e2c5c37cb22a6dd69fe5a81a9 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 21 Jun 2024 10:36:00 +0200 Subject: [PATCH 3/4] accept change in error message --- tests/testthat/_snaps/scale-hue.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/_snaps/scale-hue.md b/tests/testthat/_snaps/scale-hue.md index bccf63c43a..8221bba045 100644 --- a/tests/testthat/_snaps/scale-hue.md +++ b/tests/testthat/_snaps/scale-hue.md @@ -1,4 +1,4 @@ # scale_hue() checks the type input - `type` must be a character vector or a list of character vectors. + `type` must be a character vector or list of character vectors, not an integer vector. From d991b0ac78c47aa24da730680bb79214441418a7 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 21 Jun 2024 10:38:21 +0200 Subject: [PATCH 4/4] add news bullet --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 67c07b0b05..8b12fba15c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # ggplot2 (development version) +* (internal) improvements to `pal_qualitative()` (@teunbrand, #5013) * `geom_rug()` prints a warning when `na.rm = FALSE`, as per documentation (@pn317, #5905) * `position_dodge(preserve = "single")` now handles multi-row geoms better, such as `geom_violin()` (@teunbrand based on @clauswilke's work, #2801).