From 9faaca91d534fee4dfe4d59c41a8f61f5e3872ef Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Tue, 31 Mar 2020 18:48:37 -0300 Subject: [PATCH 1/5] fix manual limits with discrete scale that was only trained using continuous values --- R/scale-expansion.r | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/R/scale-expansion.r b/R/scale-expansion.r index d733460046..881354e593 100644 --- a/R/scale-expansion.r +++ b/R/scale-expansion.r @@ -200,10 +200,14 @@ expand_limits_continuous_trans <- function(limits, expand = expansion(0, 0), expand_limits_discrete_trans <- function(limits, expand = expansion(0, 0), coord_limits = c(NA, NA), trans = identity_trans(), range_continuous = NULL) { + if (is.discrete(limits)) { + n_discrete_limits <- length(limits) + } else { + n_discrete_limits <- 0 + } - n_limits <- length(limits) is_empty <- is.null(limits) && is.null(range_continuous) - is_only_continuous <- n_limits == 0 + is_only_continuous <- n_discrete_limits == 0 is_only_discrete <- is.null(range_continuous) if (is_empty) { @@ -211,10 +215,10 @@ expand_limits_discrete_trans <- function(limits, expand = expansion(0, 0), } else if (is_only_continuous) { expand_limits_continuous_trans(range_continuous, expand, coord_limits, trans) } else if (is_only_discrete) { - expand_limits_continuous_trans(c(1, n_limits), expand, coord_limits, trans) + expand_limits_continuous_trans(c(1, n_discrete_limits), expand, coord_limits, trans) } else { # continuous and discrete - limit_info_discrete <- expand_limits_continuous_trans(c(1, n_limits), expand, coord_limits, trans) + limit_info_discrete <- expand_limits_continuous_trans(c(1, n_discrete_limits), expand, coord_limits, trans) # don't expand continuous range if there is also a discrete range limit_info_continuous <- expand_limits_continuous_trans( From ddd81e831c9f98894b2082a65e5fbb64b8711ac1 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Tue, 31 Mar 2020 19:09:49 -0300 Subject: [PATCH 2/5] add test for the case of no discrete limits --- tests/testthat/test-scale-expansion.r | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/testthat/test-scale-expansion.r b/tests/testthat/test-scale-expansion.r index f4f0e829b5..3260442937 100644 --- a/tests/testthat/test-scale-expansion.r +++ b/tests/testthat/test-scale-expansion.r @@ -103,3 +103,14 @@ test_that("expand_limits_continuous_trans() works with inverted transformations" expect_identical(limit_info$continuous_range, c(0, 3)) expect_identical(limit_info$continuous_range_coord, c(0, -3)) }) + +test_that("expand_limits_scale_discrete() correctly handles numeric limits", { + expect_identical( + expand_limits_discrete( + -1:-16, + coord_limits = c(NA, NA), + range_continuous = c(-15, -2) + ), + c(-15, -2) + ) +}) From 5f49b8a966b5a34ba8eb209714c2766ba4690ed6 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Thu, 2 Apr 2020 10:13:55 -0300 Subject: [PATCH 3/5] add warning if user supplies continuous limits to a discrete scale --- R/scale-expansion.r | 5 ++++- tests/testthat/test-scale-expansion.r | 15 +++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/R/scale-expansion.r b/R/scale-expansion.r index 881354e593..839484cbe3 100644 --- a/R/scale-expansion.r +++ b/R/scale-expansion.r @@ -200,9 +200,12 @@ expand_limits_continuous_trans <- function(limits, expand = expansion(0, 0), expand_limits_discrete_trans <- function(limits, expand = expansion(0, 0), coord_limits = c(NA, NA), trans = identity_trans(), range_continuous = NULL) { - if (is.discrete(limits)) { + if (is.discrete(limits) || length(limits) == 0) { n_discrete_limits <- length(limits) } else { + warn( + "Continuous limits supplied to discrete scale.\nDid you mean `limits = factor(...)` or `scale_*_continuous()`?" + ) n_discrete_limits <- 0 } diff --git a/tests/testthat/test-scale-expansion.r b/tests/testthat/test-scale-expansion.r index 3260442937..a029d30347 100644 --- a/tests/testthat/test-scale-expansion.r +++ b/tests/testthat/test-scale-expansion.r @@ -105,12 +105,15 @@ test_that("expand_limits_continuous_trans() works with inverted transformations" }) test_that("expand_limits_scale_discrete() correctly handles numeric limits", { - expect_identical( - expand_limits_discrete( - -1:-16, - coord_limits = c(NA, NA), - range_continuous = c(-15, -2) + expect_warning( + expect_identical( + expand_limits_discrete( + -1:-16, + coord_limits = c(NA, NA), + range_continuous = c(-15, -2) + ), + c(-15, -2) ), - c(-15, -2) + "Continuous limits supplied to discrete scale" ) }) From d05e2efc5dceab220c45b5097c14bb57503db7d5 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Thu, 2 Apr 2020 23:08:43 -0300 Subject: [PATCH 4/5] move warning to discrete_scale() --- R/scale-.r | 10 ++++++++++ R/scale-expansion.r | 5 +---- tests/testthat/test-scale-expansion.r | 17 +++++++---------- tests/testthat/test-scales-breaks-labels.r | 4 ++-- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/R/scale-.r b/R/scale-.r index b6b56d849e..e40f4c8d68 100644 --- a/R/scale-.r +++ b/R/scale-.r @@ -164,6 +164,16 @@ discrete_scale <- function(aesthetics, scale_name, palette, name = waiver(), check_breaks_labels(breaks, labels) + if (!is.function(limits) && (length(limits) > 0) && !is.discrete(limits)) { + warn( + glue( + " + Continuous limits supplied to discrete scale. + Did you mean `limits = factor(...)` or `scale_*_continuous()`?" + ) + ) + } + position <- match.arg(position, c("left", "right", "top", "bottom")) # If the scale is non-positional, break = NULL means removing the guide diff --git a/R/scale-expansion.r b/R/scale-expansion.r index 839484cbe3..881354e593 100644 --- a/R/scale-expansion.r +++ b/R/scale-expansion.r @@ -200,12 +200,9 @@ expand_limits_continuous_trans <- function(limits, expand = expansion(0, 0), expand_limits_discrete_trans <- function(limits, expand = expansion(0, 0), coord_limits = c(NA, NA), trans = identity_trans(), range_continuous = NULL) { - if (is.discrete(limits) || length(limits) == 0) { + if (is.discrete(limits)) { n_discrete_limits <- length(limits) } else { - warn( - "Continuous limits supplied to discrete scale.\nDid you mean `limits = factor(...)` or `scale_*_continuous()`?" - ) n_discrete_limits <- 0 } diff --git a/tests/testthat/test-scale-expansion.r b/tests/testthat/test-scale-expansion.r index a029d30347..83c94c85f8 100644 --- a/tests/testthat/test-scale-expansion.r +++ b/tests/testthat/test-scale-expansion.r @@ -104,16 +104,13 @@ test_that("expand_limits_continuous_trans() works with inverted transformations" expect_identical(limit_info$continuous_range_coord, c(0, -3)) }) -test_that("expand_limits_scale_discrete() correctly handles numeric limits", { - expect_warning( - expect_identical( - expand_limits_discrete( - -1:-16, - coord_limits = c(NA, NA), - range_continuous = c(-15, -2) - ), - c(-15, -2) +test_that("expand_limits_scale_discrete() begrudgingly handles numeric limits", { + expect_identical( + expand_limits_discrete( + -1:-16, + coord_limits = c(NA, NA), + range_continuous = c(-15, -2) ), - "Continuous limits supplied to discrete scale" + c(-15, -2) ) }) diff --git a/tests/testthat/test-scales-breaks-labels.r b/tests/testthat/test-scales-breaks-labels.r index 3c8edf800a..5c180c7ec6 100644 --- a/tests/testthat/test-scales-breaks-labels.r +++ b/tests/testthat/test-scales-breaks-labels.r @@ -140,11 +140,11 @@ test_that("discrete scales with no data have no breaks or labels", { test_that("suppressing breaks, minor_breask, and labels works", { expect_equal(scale_x_continuous(breaks = NULL, limits = c(1, 3))$get_breaks(), NULL) - expect_equal(scale_x_discrete(breaks = NULL, limits = c(1, 3))$get_breaks(), NULL) + expect_equal(scale_x_discrete(breaks = NULL, limits = c("one", "three"))$get_breaks(), NULL) expect_equal(scale_x_continuous(minor_breaks = NULL, limits = c(1, 3))$get_breaks_minor(), NULL) expect_equal(scale_x_continuous(labels = NULL, limits = c(1, 3))$get_labels(), NULL) - expect_equal(scale_x_discrete(labels = NULL, limits = c(1, 3))$get_labels(), NULL) + expect_equal(scale_x_discrete(labels = NULL, limits = c("one", "three"))$get_labels(), NULL) # date, datetime lims <- as.Date(c("2000/1/1", "2000/2/1")) From c8938fafb4342965cb937b8fc38fa7233eda6965 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Fri, 3 Apr 2020 11:18:05 -0300 Subject: [PATCH 5/5] add test for continuous limits --- tests/testthat/test-scales-breaks-labels.r | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/testthat/test-scales-breaks-labels.r b/tests/testthat/test-scales-breaks-labels.r index 5c180c7ec6..cff0be54ea 100644 --- a/tests/testthat/test-scales-breaks-labels.r +++ b/tests/testthat/test-scales-breaks-labels.r @@ -138,6 +138,10 @@ test_that("discrete scales with no data have no breaks or labels", { expect_equal(sc$get_limits(), c(0, 1)) }) +test_that("passing continuous limits to a discrete scale generates a warning", { + expect_warning(scale_x_discrete(limits = 1:3), "Continuous limits supplied to discrete scale") +}) + test_that("suppressing breaks, minor_breask, and labels works", { expect_equal(scale_x_continuous(breaks = NULL, limits = c(1, 3))$get_breaks(), NULL) expect_equal(scale_x_discrete(breaks = NULL, limits = c("one", "three"))$get_breaks(), NULL)