Try coerce ordered in vec_rbind0()

[teunbrand]

This PR aims to fix #5109

Briefly, it tries to cast two ordered factors to plain factors (which can be combined in vctrs). This used to throw an error when in facetted plots, you set two ordered factors as facetting variables. This now becomes a warning.

I'd really appreciate a second pair of eyes here, as I'm still somewhat confused about the order of events in trying to restart upon errors. The thing that makes me a bit uncomfortable, is that with this PR, we're only throwing warnings on vec_ptype2() errors, and no longer on vec_cast() errors (this should just throw the usual vctrs error, I think). I don't know if we're missing an important case when the warning should be thrown in vec_cast().

To quickly recap, you weren't allowed to combine two ordered factors with incompatible levels, even though we should be able to. You also aren't allowed to combine factors and dates, which should be the case.

# For reprex purposes we want to show all warnings
options(lifecycle_verbosity = "warning")

# Legal combination
ggplot2:::vec_rbind0(
  data.frame(a = factor(c("A", "B"), ordered = TRUE)), 
  data.frame(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: )
#> Error:
#> ! Can't combine `..1$a` <factor<a022a>> and `..2$a` <ordered<fe399>>.

# Illegal combination
ggplot2:::vec_rbind0(
  data.frame(a = factor(c("A", "B"), ordered = TRUE)), 
  data.frame(a = Sys.Date() + 0:1)
)
#> Warning: Combining variables of class <ordered> and <Date> was deprecated in ggplot2
#> 3.4.0.
#> ℹ Please ensure your variables are compatible before plotting (location: )
#> Error in `ggplot2:::vec_rbind0()`:
#> ! Can't combine `..1$a` <factor<a022a>> and `..2$a` <date>.

With this PR, you no longer get an error by combining two incompatible ordered factors, but you do get a warning and the correct result. Combining dates and factors is still prohibited.

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

# Legal combination
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: )
#>   a
#> 1 A
#> 2 B
#> 3 B
#> 4 C

# Illegal combination
vec_rbind0(
  data_frame0(a = factor(c("A", "B"), ordered = TRUE)), 
  data_frame0(a = Sys.Date() + 0:1)
)
#> Warning: Combining variables of class <ordered> and <Date> was deprecated in ggplot2
#> 3.4.0.
#> ℹ Please ensure your variables are compatible before plotting (location: )
#> Error in `vec_rbind0()`:
#> ! Can't combine `..1$a` <factor<a022a>> and `..2$a` <date>.

^{Created on 2023-01-07 with reprex v2.0.2}

[thomasp85]

Can you explain about the reasoning for this guard? Why is it suddenly necessary?

[teunbrand]

This was more out of practicality than necessity. Without this, combining two ordered factors would throw 2 warnings, once during vec_ptype2() and once during vec_cast(). I don't know if there is a situation where vec_cast() should warn whereas vec_ptype2() shouldn't. See also here: #5109 (comment)

[thomasp85]

hmm... maybe keep track of whether the input is two ordered vectors and only in that case have the guard...

Maybe it is overdoing it but since I can't figure out the repercussions I'd rather be on the safe side

[teunbrand]

I too feel uncomfortable about this guard, so I'm glad you're thinking along. I think the order of events is as follows:

1. vec_ptype2(x, y) searches for a common, finalised class to coerce x and y to. Let's call that finalised class w.
2. vec_cast(x, w) coerces x to w.
3. vec_cast(y, w) coerces y to w

So if x and y are ordered factors, w will be a factor. The case where we only have two ordered factors is step (1). So, step (2) and (3) will still emit a warning about converting ordered to factors. The ideal situation would be to emit a warning at (1) and then silence (2) and (3). However, the 'track whether x and y are both ordered factors'-rule can't be applied in (2) and (3) because w is a regular factor.

I'll puzzle around a bit more to see what might be done, but it'll probably become more complicated than just tracking ordered factors.

[teunbrand]

Alright, should be safer now, though it has become a little bit more complex than I'd hoped for.

[thomasp85]

hmm... I must admit I'm now closer to just accepting a double warning rather than complicating the restart handler in this way - it feels like dangerous over-engineering for a situation that arrises so infrequently. Thoughts?

[teunbrand]

Yeah I agree with you that this might be over-complicated for little practical purposes other than displaying a slightly more comprehensible set of warnings. Also, these are lifecycle warnings, so they're only displayed once every 8 hours under default settings. The important bit is turning the original error into a warning so that the plot in #5109 at least renders (which is an easy fix).

If the warnings become a pertinent problem, which seems unlikely, we could always decide to fix these at some later point.

[thomasp85]

Agreed - let's roll back to simply avoiding the error

[teunbrand]

Alright, I've reverted everything but the error avoidance

[thomasp85]

LGTM

[thomasp85]

But please add a unit test and NEWS bullet