Can't combine ordered factors in a facet_grid

[DOSull]

When I use two ordered factors in a facet_grid, with as.table = FALSE in an attempt to get the plot to arrange the facets in a conventional increasing left-to-right, bottom-to-top grid, I get a warning and an error

Error:
! Can't combine `..1$B` <factor<0715c>> and `..2$B` <ordered<08921>>.
Run `rlang::last_error()` to see where the error occurred.
Warning message:
Combining variables of class <ordered> and <ordered> was deprecated in ggplot2 3.4.0.
ℹ Please ensure your variables are compatible before plotting (location: `join_keys()`)
This warning is displayed once every 8 hours.
Call `lifecycle::last_lifecycle_warnings()` to see where this warning was generated. ```

Here's a reprex

library(ggplot2)
library(tidyr)

data <- data.frame(
  x = rnorm(100), y = rnorm(100),
  A = factor(sample(c("tall", "grande", "venti"), 100, replace = TRUE), 
             levels = c("tall", "grande", "venti"), ordered = TRUE),
  B = factor(sample(c("L", "M", "S"), 100, replace = TRUE),
             levels = c("S", "M", "L"), ordered = TRUE))

ggplot(data) + 
  geom_point(aes(x = x, y = y)) +
  facet_grid(B ~ A, as.table = TRUE)

# FAILS
ggplot(data) + 
  geom_point(aes(x = x, y = y)) +
  facet_grid(B ~ A, as.table = FALSE)

This seems like a regression in this particular context, since it gives a lot less control over the appearance of the arrangement of the grid of small multiples, even when I am 'doing the right thing' and ordering my factors to get specific effects. Perhaps it was introduced to fix some other problem, but I can't see what it accomplishes in this context.

The error message seems to be from the function utilities::with_ordered_restart here

ggplot2/R/utilities.r

Line 664 in 63125db

msg <- paste0("Combining variables of class <", class_x, "> and <", class_y, ">")

but it's not clear to me why it should be treated as an error in what is surely a common use-case.

Possibly related to #4180

[yutannihilation]

Thanks, confirmed. Looking at this comment on the function, maybe this behavior is not intended...?

ggplot2/R/utilities.r

Lines 617 to 619 in 63125db

	# Restart handler for using vec_rbind with mix of types
	# Ordered is coerced to factor
	# If a character vector is present the other is converted to character

[thomasp85]

In general there is no meaningful way to combine two ordered vectors that doesn't share the same levels since an ordering of the two sets of levels cannot be deduced. That being said I thought we would have just thrown a warning and not an error for now

[yutannihilation]

That being said I thought we would have just thrown a warning and not an error for now

To me, that was the intention, considering the comment says "Ordered is coerced to factor" and there's actually a warning:

Warning message:
Combining variables of class <ordered> and <ordered> was deprecated in ggplot2 3.4.0.
ℹ Please ensure your variables are compatible before plotting (location: `join_keys()`)
This warning is displayed once every 8 hours.
Call `lifecycle::last_lifecycle_warnings()` to see where this warning was generated. 

The problem is that the actual code fail to coerce; it converts one of the ordered to a factor, but not both, which ends up with the vctrs error (! Can't combine ..1$B<factor<0715c>> and..2$B <ordered<08921>>.).

ggplot2/R/utilities.r

Lines 632 to 638 in 63125db

	if (is.ordered(x) || is.ordered(y)) {
	restart <- TRUE
	if (is.ordered(x)) {
	x <- factor(as.character(x), levels = levels(x))
	} else {
	y <- factor(as.character(y), levels = levels(y))
	}

Am I wrong...? Sorry, it might be just I'm reading the code wrong.

[DOSull]

I really don't understand why an ordered factor needs to be converted to unordered to be applied as a conditioning variable in a facet grid. Some order will still be applied in the end (alphabetical?) -- how is that different from using the user-supplied order of the original data, other than that it will annoy users who thought they had specified the order of their factors?

In any case, if the intention of the above code snippet is that both factors be converted to unordered, then it should be

 if (is.ordered(x) || is.ordered(y)) { 
   restart <- TRUE 
   if (is.ordered(x)) { 
     x <- factor(as.character(x), levels = levels(x)) 
   } 
   if (is.ordered(y)) { 
     y <- factor(as.character(y), levels = levels(y)) 
   }
 } 

which would de-order both factors. That seems wrong to me, but if that's the intention then per @yutannihilation the code seems incorrect.

[clauswilke]

To be clear, whether a factor is ordered or not has no bearing on how ggplot2 treats it when plotting. It uses the order of the levels regardless, even for unordered factors. Therefore, ordering should probably just be stripped from the factor during data processing, and maybe even issuing a warning is not necessary.

And yes, the current code is clearly wrong, as it ignores the possibility that both factors are ordered.

library(ggplot2)
library(tidyr)

data <- data.frame(
  x = rnorm(100), y = rnorm(100),
  A = factor(sample(c("tall", "grande", "venti"), 100, replace = TRUE), 
             levels = c("tall", "grande", "venti"), ordered = FALSE),
  B = factor(sample(c("L", "M", "S"), 100, replace = TRUE),
             levels = c("S", "M", "L"), ordered = FALSE))

# works as expected
ggplot(data) + 
  geom_point(aes(x = x, y = y)) +
  facet_grid(B ~ A, as.table = TRUE)

# works as expected
ggplot(data) + 
  geom_point(aes(x = x, y = y)) +
  facet_grid(B ~ A, as.table = FALSE)

^{Created on 2022-12-14 by the reprex package (v2.0.1)}

[DOSull]

So will this get fixed in the next release?

[teunbrand]

Hi @DOSull, are you perhaps interested in submitting a PR for this? I think you already figured out the code in #5109 (comment). No worries if you're not interested, then we'll fix it at some point.

[DOSull]

I don't know how the whole business of PRs and so on works, and I don't have familiarity with the code base---my suggested fix just follows @yutannihilation's identification of the issue! Thanks for thinking of me (!) but I should probably leave it to the experts.

[teunbrand]

Well if you would have been curious about it, you can find some (slightly outdated) contributing guidelines here. There are also a bunch of helper functions in the {usethis} package, and they have a good article detailing how submit a PR.

[teunbrand]

This turned out to be slightly more complicated than I'd anticipated. With the proposed changed, we get:

devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2

vec_rbind0(
  data_frame0(a = factor(c("A", "B"), ordered = TRUE)), 
  data_frame0(a = factor(c("B", "C"), ordered = TRUE))
)
#> Warning: Combining variables of class <ordered> and <ordered> was deprecated in ggplot2
#> 3.4.0.
#> ℹ Please ensure your variables are compatible before plotting (location: )
#> Warning: Combining variables of class <ordered> and <factor> was deprecated in ggplot2
#> 3.4.0.
#> ℹ Please ensure your variables are compatible before plotting (location: )
#>   a
#> 1 A
#> 2 B
#> 3 B
#> 4 C

^{Created on 2022-12-27 by the reprex package (v2.0.1)}

Where the output is correct and no error is thrown. However, there are two warnings. One for the vec_ptype2() part which I think should be thrown, and one for the vec_cast() part which seems redundant. I wouldn't know how to elegantly solve the two warning bit.