Skip to content

Commit da60e8f

Browse files
authored
Prevent mismapping when limits contain NA in non-last position (#5848)
* protect against early NAs * add test * add news bullet * fix typos * simplify by discarding `NA` limits
1 parent 4eb6587 commit da60e8f

File tree

3 files changed

+29
-1
lines changed

3 files changed

+29
-1
lines changed

NEWS.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
# ggplot2 (development version)
22

3+
* Fixed bug where `na.value` was incorrectly mapped to non-`NA` values
4+
(@teunbrand, #5756).
35
* Fixed bug in `guide_custom()` that would throw error with `theme_void()`
46
(@teunbrand, #5856).
57
* New helper function `ggpar()` to translate ggplot2's interpretation of

R/scale-.R

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -947,7 +947,8 @@ ScaleDiscrete <- ggproto("ScaleDiscrete", Scale,
947947
transform = identity,
948948

949949
map = function(self, x, limits = self$get_limits()) {
950-
n <- sum(!is.na(limits))
950+
limits <- limits[!is.na(limits)]
951+
n <- length(limits)
951952
if (n < 1) {
952953
return(rep(self$na.value, length(x)))
953954
}

tests/testthat/test-scales.R

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -730,3 +730,28 @@ test_that("Discrete scales with only NAs return `na.value`", {
730730
sc$train(x)
731731
expect_equal(sc$map(x), c(NA_real_, NA_real_))
732732
})
733+
734+
test_that("discrete scales work with NAs in arbitrary positions", {
735+
# Prevents intermediate caching of palettes
736+
map <- function(x, limits) {
737+
sc <- scale_colour_manual(
738+
values = c("red", "green", "blue"),
739+
na.value = "gray"
740+
)
741+
sc$map(x, limits)
742+
}
743+
744+
# All inputs should yield output regardless of where NA is
745+
input <- c("A", "B", "C", NA)
746+
output <- c("red", "green", "blue", "gray")
747+
748+
test <- map(input, limits = c("A", "B", "C", NA))
749+
expect_equal(test, output)
750+
751+
test <- map(input, limits = c("A", NA, "B", "C"))
752+
expect_equal(test, output)
753+
754+
test <- map(input, limits = c(NA, "A", "B", "C"))
755+
expect_equal(test, output)
756+
757+
})

0 commit comments

Comments
 (0)