From 5ec9f966d1dd511dbfed95583041f0d41a808dcf Mon Sep 17 00:00:00 2001 From: Balazs Banfai Date: Wed, 7 Jul 2021 10:15:33 +0200 Subject: [PATCH 1/4] Remove extraneous limit values with drop=TRUE * issue introduced in #4471 * potential solution for #4534 and #4511 * add extra tests --- NEWS.md | 3 +++ R/scale-.r | 6 +++++- tests/testthat/test-scale-manual.r | 26 +++++++++++++++++++++----- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/NEWS.md b/NEWS.md index 67e41b9427..fe48000acc 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # ggplot2 (development version) +* Fixes issue introduced in #4471 with extra values supplied to manual scale + showing up in legend (@banfai, #4511, #4534) + # ggplot2 3.3.5 This is a very small release focusing on fixing a couple of untenable issues that surfaced with the 3.3.4 release diff --git a/R/scale-.r b/R/scale-.r index 3fa2f6cbde..e73089825d 100644 --- a/R/scale-.r +++ b/R/scale-.r @@ -496,7 +496,11 @@ Scale <- ggproto("Scale", NULL, } else if (is.function(self$limits)) { self$limits(self$range$range) } else { - self$limits + if (!is.null(self$drop) && self$drop && !is.null(self$range$range)) { + intersect(self$range$range, self$limits) + } else { + self$limits + } } }, diff --git a/tests/testthat/test-scale-manual.r b/tests/testthat/test-scale-manual.r index b8af871e99..4188527c33 100644 --- a/tests/testthat/test-scale-manual.r +++ b/tests/testthat/test-scale-manual.r @@ -1,9 +1,13 @@ context("scale_manual") test_that("names of values used in manual scales", { - s <- scale_colour_manual(values = c("8" = "c","4" = "a","6" = "b")) - s$train(c("4", "6", "8")) - expect_equal(s$map(c("4", "6", "8")), c("a", "b", "c")) + s1 <- scale_colour_manual(values = c("8" = "c", "4" = "a", "6" = "b")) + s1$train(c("4", "6", "8")) + expect_equal(s1$map(c("4", "6", "8")), c("a", "b", "c")) + + s2 <- scale_colour_manual(values = c("8" = "c", "4" = "a", "6" = "b"), na.value = NA) + s2$train(c("4", "8")) + expect_equal(s2$map(c("4", "6", "8")), c("a", NA, "c")) }) @@ -87,13 +91,25 @@ test_that("unnamed values match breaks in manual scales", { }) test_that("limits works (#3262)", { - # named charachter vector + # named character vector s1 <- scale_colour_manual(values = c("8" = "c", "4" = "a", "6" = "b"), limits = c("4", "8"), na.value = NA) s1$train(c("4", "6", "8")) expect_equal(s1$map(c("4", "6", "8")), c("a", NA, "c")) - # named charachter vector + # unnamed character vector s2 <- scale_colour_manual(values = c("c", "a", "b"), limits = c("4", "8"), na.value = NA) s2$train(c("4", "6", "8")) expect_equal(s2$map(c("4", "6", "8")), c("c", NA, "a")) }) + +test_that("fewer values (#3451)", { + # named character vector + s1 <- scale_colour_manual(values = c("4" = "a", "8" = "c"), na.value = NA) + s1$train(c("4", "6", "8")) + expect_equal(s1$map(c("4", "6", "8")), c("a", NA, "c")) + + # unnamed character vector + s2 <- scale_colour_manual(values = c("4", "8"), na.value = NA) + s2$train(c("4", "6", "8")) + expect_error(s2$map(c("4", "6", "8")), "Insufficient values") +}) From d93f12a74e9fad8128196a916680d1a40916fd43 Mon Sep 17 00:00:00 2001 From: Balazs Banfai Date: Sat, 18 Sep 2021 20:50:10 +0200 Subject: [PATCH 2/4] Revert change to Scale --- R/scale-.r | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/R/scale-.r b/R/scale-.r index e73089825d..3fa2f6cbde 100644 --- a/R/scale-.r +++ b/R/scale-.r @@ -496,11 +496,7 @@ Scale <- ggproto("Scale", NULL, } else if (is.function(self$limits)) { self$limits(self$range$range) } else { - if (!is.null(self$drop) && self$drop && !is.null(self$range$range)) { - intersect(self$range$range, self$limits) - } else { - self$limits - } + self$limits } }, From f9e9a76c770854202b650e201bfbafb52878c24a Mon Sep 17 00:00:00 2001 From: Balazs Banfai Date: Sat, 18 Sep 2021 20:50:27 +0200 Subject: [PATCH 3/4] Add more tests --- tests/testthat/test-scale-manual.r | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/testthat/test-scale-manual.r b/tests/testthat/test-scale-manual.r index 4188527c33..d33fe17da8 100644 --- a/tests/testthat/test-scale-manual.r +++ b/tests/testthat/test-scale-manual.r @@ -113,3 +113,31 @@ test_that("fewer values (#3451)", { s2$train(c("4", "6", "8")) expect_error(s2$map(c("4", "6", "8")), "Insufficient values") }) + +test_that("limits and breaks (#4619)", { + # values don't change legend order + s1 <- scale_colour_manual( + values = c("8" = "c", "4" = "a", "6" = "b"), + ) + s1$train(c("8", "6", "4")) + expect_equal(s1$map(c("8", "6", "4")), c("c", "b", "a")) + expect_equal(s1$break_positions(), c("a", "b", "c")) + + # limits change legend order + s2 <- scale_colour_manual( + values = c("8" = "c", "4" = "a", "6" = "b", "0" = "x"), + limits = c("0", "4", "6", "8") + ) + s2$train(c("8", "6", "4")) + expect_equal(s2$map(c("4", "6", "8")), c("a", "b", "c")) + expect_equal(s2$break_positions(), c("x", "a", "b", "c")) + + # breaks work + s3 <- scale_colour_manual( + values = c("8" = "c", "4" = "a", "6" = "b"), + breaks = c("4", "8") + ) + s3$train(c("4", "6", "8")) + expect_equal(s3$map(c("4", "6", "8")), c("a", "b", "c")) + expect_equal(s3$break_positions(), c("a", "c")) +}) From d03a6b9b0e00b72de78a617d57dbe2e175fc05c7 Mon Sep 17 00:00:00 2001 From: Balazs Banfai <5557093+banfai@users.noreply.github.com> Date: Sat, 18 Sep 2021 21:12:14 +0200 Subject: [PATCH 4/4] Revert changes to NEWS.md --- NEWS.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/NEWS.md b/NEWS.md index 058e598db9..4a5398a300 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,8 +1,5 @@ # ggplot2 (development version) -* Fixes issue introduced in #4471 with extra values supplied to manual scale - showing up in legend (@banfai, #4511, #4534) - # ggplot2 3.3.5 This is a very small release focusing on fixing a couple of untenable issues that surfaced with the 3.3.4 release