From 0867837cfef7afb97ab30aa8bdbc7080494a19cd Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 31 May 2024 12:58:38 +0200 Subject: [PATCH 1/5] dodge by max number of groups per panel/position --- R/position-jitterdodge.R | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/R/position-jitterdodge.R b/R/position-jitterdodge.R index fba28a47fa..a9f26a33ef 100644 --- a/R/position-jitterdodge.R +++ b/R/position-jitterdodge.R @@ -47,17 +47,9 @@ PositionJitterdodge <- ggproto("PositionJitterdodge", Position, flipped_aes <- has_flipped_aes(data) data <- flip_data(data, flipped_aes) width <- self$jitter.width %||% (resolution(data$x, zero = FALSE, TRUE) * 0.4) - # Adjust the x transformation based on the number of 'dodge' variables - possible_dodge <- c("fill", "colour", "linetype", "shape", "size", "alpha") - dodgecols <- intersect(possible_dodge, colnames(data)) - if (length(dodgecols) == 0) { - cli::cli_abort(c( - "{.fn position_jitterdodge} requires at least one aesthetic to dodge by.", - i = "Use one of {.or {.val {possible_dodge}}} aesthetics." - )) - } - ndodge <- lapply(data[dodgecols], levels) # returns NULL for numeric, i.e. non-dodge layers - ndodge <- vec_unique_count(unlist(ndodge)) + + ndodge <- split(data$group, list(data$PANEL, data$x)) + ndodge <- max(vapply(ndodge, vec_unique_count, integer(1))) list( dodge.width = self$dodge.width, From 6af22c92546e9305450233832047b126bb401286 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 31 May 2024 13:03:49 +0200 Subject: [PATCH 2/5] fill missing defaults --- R/position-jitterdodge.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/position-jitterdodge.R b/R/position-jitterdodge.R index a9f26a33ef..dc58c27694 100644 --- a/R/position-jitterdodge.R +++ b/R/position-jitterdodge.R @@ -52,8 +52,8 @@ PositionJitterdodge <- ggproto("PositionJitterdodge", Position, ndodge <- max(vapply(ndodge, vec_unique_count, integer(1))) list( - dodge.width = self$dodge.width, - jitter.height = self$jitter.height, + dodge.width = self$dodge.width %||% 0.75, + jitter.height = self$jitter.height %||% 0, jitter.width = width / (ndodge + 2), seed = self$seed, flipped_aes = flipped_aes From 2592333a3815d6803cd9cac83a0f0ee653826139 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 31 May 2024 13:04:09 +0200 Subject: [PATCH 3/5] `position_jitterdodge()` doesn't fail this test-case anymore --- tests/testthat/test-position-jitterdodge.R | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/testthat/test-position-jitterdodge.R b/tests/testthat/test-position-jitterdodge.R index 49e8378666..fb3274e61a 100644 --- a/tests/testthat/test-position-jitterdodge.R +++ b/tests/testthat/test-position-jitterdodge.R @@ -1,8 +1,3 @@ -test_that("position_jitterdodge() fails with meaningful error", { - p <- ggplot(mtcars) + geom_point(aes(disp, mpg), position = 'jitterdodge') - expect_snapshot_error(ggplot_build(p)) -}) - test_that("position_jitterdodge preserves widths", { ld <- layer_data( ggplot(mtcars, aes(factor(cyl), fill = factor(am))) + From 6cb629981cf3c6241849f2800544ff8ff69d9dc0 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 31 May 2024 13:05:02 +0200 Subject: [PATCH 4/5] add news bullet --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index de3e87cee3..3d0ba28e83 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # ggplot2 (development version) +* `position_jitterdodge()` now dodges by `group` (@teunbrand, #3656) * The `arrow.fill` parameter is now applied to more line-based functions: `geom_path()`, `geom_line()`, `geom_step()` `geom_function()`, line geometries in `geom_sf()` and `element_line()`. From 151754651fb914e3824691f118e4bfdfc6e31bb1 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 3 Jun 2024 17:00:34 +0200 Subject: [PATCH 5/5] Copy faster approach from #5928 --- R/position-jitterdodge.R | 5 +++-- tests/testthat/_snaps/position-jitterdodge.md | 8 -------- 2 files changed, 3 insertions(+), 10 deletions(-) delete mode 100644 tests/testthat/_snaps/position-jitterdodge.md diff --git a/R/position-jitterdodge.R b/R/position-jitterdodge.R index dc58c27694..768f3d8385 100644 --- a/R/position-jitterdodge.R +++ b/R/position-jitterdodge.R @@ -48,8 +48,9 @@ PositionJitterdodge <- ggproto("PositionJitterdodge", Position, data <- flip_data(data, flipped_aes) width <- self$jitter.width %||% (resolution(data$x, zero = FALSE, TRUE) * 0.4) - ndodge <- split(data$group, list(data$PANEL, data$x)) - ndodge <- max(vapply(ndodge, vec_unique_count, integer(1))) + ndodge <- vec_unique(data[c("group", "PANEL", "x")]) + ndodge <- vec_group_id(ndodge[c("PANEL", "x")]) + ndodge <- max(tabulate(ndodge, attr(ndodge, "n"))) list( dodge.width = self$dodge.width %||% 0.75, diff --git a/tests/testthat/_snaps/position-jitterdodge.md b/tests/testthat/_snaps/position-jitterdodge.md deleted file mode 100644 index 1a387e880e..0000000000 --- a/tests/testthat/_snaps/position-jitterdodge.md +++ /dev/null @@ -1,8 +0,0 @@ -# position_jitterdodge() fails with meaningful error - - Problem while computing position. - i Error occurred in the 1st layer. - Caused by error in `setup_params()`: - ! `position_jitterdodge()` requires at least one aesthetic to dodge by. - i Use one of "fill", "colour", "linetype", "shape", "size", or "alpha" aesthetics. -