stricter test in find_x_overlaps() (#2615)

[cpsievert]

No description provided.

[karawoo]

Thanks for looking into this @cpsievert! With this fix the box plot succeeds silently, whereas in the CRAN version it succeeds with a warning. @hadley do you think we should try to keep the warning behavior?

library("ggplot2") # CRAN
ggplot(PlantGrowth, aes(x = group, y = weight)) + 
  geom_boxplot() +
  scale_x_discrete(limits = c("trt1", "ctrl"))
#> Warning: Removed 10 rows containing non-finite values (stat_boxplot).

Created on 2018-05-16 by the reprex package (v0.2.0).

[hadley]

I think we should try and fix the warning - I think that's probably an alternative to @cpsievert's solution - i.e. the missing values should be removed before we get to dodge2.

[karawoo]

Actually I was wrong, with this PR there's a warning about 1 missing row, and that's in line with the behavior of position = "dodge" on master:

devtools::install_github("tidyverse/ggplot2", quiet = TRUE)
library("ggplot2")
ggplot(PlantGrowth, aes(x = group, y = weight)) + 
  geom_boxplot(position = "dodge") +
  scale_x_discrete(limits = c("trt1", "ctrl"))
#> Warning: Removed 1 rows containing missing values (geom_boxplot).

Created on 2018-05-16 by the reprex package (v0.2.0).

This is still a bit odd because there are 10 rows of data that are removed, but this change in behavior isn't related to find_x_overlaps() so I think we should merge this fix.

However it turns up another issue we need to address separately, which is that the widths of the boxes is messed up 😭:

devtools::install_github("cpsievert/ggplot2", ref = "fix_x_overlaps", quiet = TRUE)
library("ggplot2")
ggplot(PlantGrowth, aes(x = group, y = weight)) + 
  geom_boxplot() +
  scale_x_discrete(limits = c("trt1", "ctrl"))
#> Warning: Removed 1 rows containing missing values (geom_boxplot).

Created on 2018-05-16 by the reprex package (v0.2.0).

[hadley]

I'm not sure that the fix is correct - what should counter be if xmin or xmax is NA?

[karawoo]

Looked into this some more and I propose we add

data <- remove_missing(data, na.rm = FALSE, vars = "x", name = "geom_boxplot")

here:

ggplot2/R/stat-boxplot.r

Lines 47 to 50 in 4b609fb

	setup_data = function(data, params) {
	data$x <- data$x %||% 0
	data
	},

Which results in:

1. plot succeeds with a warning about 10 removed rows (not 1), for both versions of position dodge
2. box widths are then correct (phew)

@hadley if you agree I can submit a PR and test tomorrow, unless @cpsievert wants to add it here instead.

[hadley]

Yes, that sounds perfect!

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/