Facets now don't fail if faceting vars are COL, ROW, etc.

[eliocamp]

This will close #4138.

Works around the issue of duplicated column names by explicitly selecting either data columns or layout columns in the layout data.frame when needed. Added tests that pass.

reprex :)

library(ggplot2)
mtcars$PANEL <- mtcars$cyl


ggplot(mtcars, aes(hp, mpg)) +
  geom_point() +
  facet_grid(cyl ~  am) 

ggplot(mtcars, aes(hp, mpg)) +
  geom_point() +
  facet_grid(PANEL ~  am) 

^{Created on 2021-01-12 by the reprex package (v0.3.0)}

[yutannihilation]

Thanks for tackling the issue. While this work around looks good at the moment, I'm wondering if the implicit assumption is and will be always guaranteed to be true; this assumes the structure of the result of compute_layout() are preserved until map_data() including:

1. the column order
2. the existence of duplicated columns

Especially regarding point 2, ggplot2 at least rejects data with duplicated columns, so I feel we should avoid using duplicated columns even if it's the internal data in order to reduce the maintenance costs.

library(ggplot2)

d <- data.frame(a = 1:10)
d2 <- cbind(d, d)
ggplot(d2) + geom_bar(aes(a))
#> Error: `data` must be uniquely named but has duplicate columns

But, honestly I don't know about the internals very well, so I might be wrong. I'll wait for other maintainers' comments.

[eliocamp]

I don't have much insight. It seems to me that ggplot2 controls the layout dataframe completely, so unless something changes in the future, then it shouldn't be an issue.

I do hear your point that it would be safer to not have duplicated columns at all, but insofar that the internals joins a dataframe with what are essentially user-defined column, I don't see how it can be changed easily. :shrug: . One fix would be to make layout a list with elements "layout" and "data" which separate the data columns from the layout columns. The issue there is to be 100% sure to always update both dataframes when filtering or ordering, and that it will surely involve a lot more work on the internals.

[teunbrand]

I'm closing this in favour of #4922. I think the error is sufficient to alert users to what is going on. Thanks for the pull request though!