From 1d80db220bd90ce74132de102c243ed60ed68f12 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Tue, 13 Apr 2021 13:02:03 +0200 Subject: [PATCH 1/4] calculate offset within each stackaxis --- NEWS.md | 3 +++ R/geom-dotplot.r | 14 +++++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index b38aa88aed..60159e47bf 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # ggplot2 (development version) +* Fix bug in `geom_dotplot()` where dots would be positioned wrong with + `stackgroups = TRUE` (@thomasp85, #1745) + * Make sure position_jitter creates the same jittering independent of whether it is called by name or with constructor (@thomasp85, #2507) diff --git a/R/geom-dotplot.r b/R/geom-dotplot.r index 1c82f374c9..5d1a42fc0a 100644 --- a/R/geom-dotplot.r +++ b/R/geom-dotplot.r @@ -208,25 +208,29 @@ GeomDotplot <- ggproto("GeomDotplot", Geom, stackdots <- function(a) a - 1 - floor(max(a - 1) / 2) stackaxismin <- -.5 stackaxismax <- .5 + } else { + abort(glue('{params$stackdir} is not a recognized value for `stackdir`. Use "up", "down", "center", or "centerwhole"')) } - # Fill the bins: at a given x (or y), if count=3, make 3 entries at that x data <- data[rep(1:nrow(data), data$count), ] # Next part will set the position of each dot within each stack # If stackgroups=TRUE, split only on x (or y) and panel; if not stacking, also split by group plyvars <- params$binaxis %||% "x" + stackaxis <- setdiff(c("x", "y"), plyvars) plyvars <- c(plyvars, "PANEL") if (is.null(params$stackgroups) || !params$stackgroups) plyvars <- c(plyvars, "group") + plyvars <- c(plyvars, stackaxis) + # Within each x, or x+group, set countidx=1,2,3, and set stackpos according to stack function data <- dapply(data, plyvars, function(xx) { - xx$countidx <- 1:nrow(xx) - xx$stackpos <- stackdots(xx$countidx) - xx - }) + xx$countidx <- 1:nrow(xx) + xx$stackpos <- stackdots(xx$countidx) + xx + }) # Set the bounding boxes for the dots From 13d6f074654437cb8c516265cf8c58d254e751cb Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Tue, 13 Apr 2021 15:24:05 +0200 Subject: [PATCH 2/4] fix regressions --- R/geom-dotplot.r | 4 +- tests/figs/deps.txt | 2 +- ...-stackgroups-histodot-currently-broken.svg | 102 +++++++++--------- 3 files changed, 55 insertions(+), 53 deletions(-) diff --git a/R/geom-dotplot.r b/R/geom-dotplot.r index 5d1a42fc0a..92e5e27138 100644 --- a/R/geom-dotplot.r +++ b/R/geom-dotplot.r @@ -223,7 +223,9 @@ GeomDotplot <- ggproto("GeomDotplot", Geom, if (is.null(params$stackgroups) || !params$stackgroups) plyvars <- c(plyvars, "group") - plyvars <- c(plyvars, stackaxis) + if (stackaxis == "x") { + plyvars <- c(plyvars, "x") + } # Within each x, or x+group, set countidx=1,2,3, and set stackpos according to stack function data <- dapply(data, plyvars, function(xx) { diff --git a/tests/figs/deps.txt b/tests/figs/deps.txt index 8d7f66b6a1..1fcd925731 100644 --- a/tests/figs/deps.txt +++ b/tests/figs/deps.txt @@ -1,3 +1,3 @@ - vdiffr-svg-engine: 1.0 - vdiffr: 0.3.3 -- freetypeharfbuzz: 0.2.5 +- freetypeharfbuzz: 0.2.6 diff --git a/tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot-currently-broken.svg b/tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot-currently-broken.svg index ef53f1c0f0..46b2da6486 100644 --- a/tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot-currently-broken.svg +++ b/tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot-currently-broken.svg @@ -19,96 +19,96 @@ - - - - - - - - - - - - + + + + + + + + + + + + - - - - + + - - - + + + + - - + + + + - - + + - - + + - - + + + - - + - + - - - + + - - + + - - - - + + + + + - - + - - - - - + + + + + - - - + + + + - - + From 9f8a49c0e19eec78a013e371581469853ac24a6a Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Tue, 13 Apr 2021 15:29:41 +0200 Subject: [PATCH 3/4] fix name of visual test to indicate it is no longer broken --- ...ntly-broken.svg => bin-y-dodging-3-stackgroups-histodot.svg} | 2 +- tests/testthat/test-geom-dotplot.R | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename tests/figs/geom-dotplot/{bin-y-dodging-3-stackgroups-histodot-currently-broken.svg => bin-y-dodging-3-stackgroups-histodot.svg} (99%) diff --git a/tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot-currently-broken.svg b/tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot.svg similarity index 99% rename from tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot-currently-broken.svg rename to tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot.svg index 46b2da6486..b008396488 100644 --- a/tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot-currently-broken.svg +++ b/tests/figs/geom-dotplot/bin-y-dodging-3-stackgroups-histodot.svg @@ -141,5 +141,5 @@ A B -bin y, dodging, 3 stackgroups, histodot (currently broken) +bin y, dodging, 3 stackgroups, histodot diff --git a/tests/testthat/test-geom-dotplot.R b/tests/testthat/test-geom-dotplot.R index e35bca06c7..1368d781c4 100644 --- a/tests/testthat/test-geom-dotplot.R +++ b/tests/testthat/test-geom-dotplot.R @@ -207,7 +207,7 @@ test_that("geom_dotplot draws correctly", { # This one is currently broken but it would be a really rare case, and it # probably requires a really ugly hack to fix - expect_doppelganger("bin y, dodging, 3 stackgroups, histodot (currently broken)", + expect_doppelganger("bin y, dodging, 3 stackgroups, histodot", ggplot(dat2, aes(x, y, fill = g)) + geom_dotplot(binaxis = "y", binwidth = .25, stackgroups = TRUE, method = "histodot", alpha = 0.5, stackdir = "centerwhole") From aa1393520f45b8b2d3812bfa33a3d50f95a6ce62 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 14 Apr 2021 08:08:31 +0200 Subject: [PATCH 4/4] use arg_match0 in constructor --- R/geom-dotplot.r | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/R/geom-dotplot.r b/R/geom-dotplot.r index 92e5e27138..9b34e517f1 100644 --- a/R/geom-dotplot.r +++ b/R/geom-dotplot.r @@ -149,6 +149,7 @@ geom_dotplot <- function(mapping = NULL, data = NULL, if (stackgroups && method == "dotdensity" && binpositions == "bygroup") message('geom_dotplot called with stackgroups=TRUE and method="dotdensity". You probably want to set binpositions="all"') + stackdir <- arg_match0(stackdir, c("up", "down", "center", "centerwhole"), "stackdir") layer( data = data, mapping = mapping, @@ -208,8 +209,6 @@ GeomDotplot <- ggproto("GeomDotplot", Geom, stackdots <- function(a) a - 1 - floor(max(a - 1) / 2) stackaxismin <- -.5 stackaxismax <- .5 - } else { - abort(glue('{params$stackdir} is not a recognized value for `stackdir`. Use "up", "down", "center", or "centerwhole"')) } # Fill the bins: at a given x (or y), if count=3, make 3 entries at that x