Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move {tibble} to suggests #5990

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix part of #5986 (comment).

Briefly, code didn't use {tibble} a lot, so these instances have been replaced with data_frame0().
I still think {tibble} has a place in Suggests rather than removed entirely, because it has a nicer print method for data such as mpg and it is used in vignettes. If we wouldn't feel strongly about these use-cases, we could also remove {tibble} altogether.

@@ -533,7 +533,7 @@ is_facets <- function(x) {
# but that seems like a reasonable tradeoff.
eval_facets <- function(facets, data, possible_columns = NULL) {
vars <- compact(lapply(facets, eval_facet, data, possible_columns = possible_columns))
data_frame0(tibble::as_tibble(vars))
data_frame0(!!!vars)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We potentially lose some tibble validation checks here, but if we would want to validate here, I think it'd be better to control the validation ourselves.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I (and probably we all) agree with you in that the validation ggplot2 wants isn't necessarily the same as the one provided by tibble (cf. #3018 (comment))! But, I feel a bit anxious that no validation will be applied here after we move tibble to suggests.

Defining a concrete spec of the validation is a tough job and takes time. So, it might be realistic to keep relying on tibble at the moment. I think we can assume an ordinary user uses tibble anyway via tidyverse.

Copy link
Collaborator Author

@teunbrand teunbrand Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a bit anxious that no validation will be applied here

Yeah I empathise with this, but let me try to convince you this change wouldn't make the situation any worse than it currently is.

tibble()'s validation is essentially vec_is(column) || is.expression(column) in tibble:::is_valid_col() combined with tibble:::make_valid_col(), which is just turning the expression into a list so that it complies with {vctrs}.

The validation in ggplot2:::data_frame0() also has the vec_is() constraint (via vctrs C internals I presume), but not the expression coersion. Seeing as we don't really accept expression columns as facet specificiation, this isn't a loss.

In addition to the similarity, the current validation isn't constraining the data types to the ones we can use as facet variables. Current validation happily accepts matrix or list columns which we then struggle to use downstream:

library(ggplot2)
df <- mpg
df$mtx <- matrix(1:4, nrow = 1, ncol = 4)
df$lst <- list(4)

p <- ggplot(df, aes(displ, hwy)) +
  geom_point()
p + facet_wrap(~mtx)
#> Warning in `[<-.data.frame`(`*tmp*`, , value = list(mtx = structure(c(1L, :
#> replacement element 1 has 936 rows to replace 234 rows
#> Warning in `[<-.data.frame`(`*tmp*`, , value = list(PANEL = structure(1:4,
#> levels = c("1", : replacement element 4 has 16 rows to replace 4 rows
#> Warning in mat[index] <- unlist(unname(strips), recursive = FALSE)[[position]]:
#> number of items to replace is not a multiple of replacement length

p + facet_wrap(~lst)
#> Error in sort.int(x, na.last = na.last, decreasing = decreasing, ...): 'x' must be atomic

Created on 2024-07-10 with reprex v2.1.0

Copy link
Member

@yutannihilation yutannihilation Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the details. Then, I think I'm now convinced! The issue I referred to was before the vctrs integration, so it seems I was outdated...

I now feel this pull request is a go, but I'd like to wait for another approval to make sure.

Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The inclusion of tibble was indeed before we had vctrs integration and it was just forgotten when we adopted the package wholesale

@teunbrand teunbrand merged commit 8bb9641 into tidyverse:main Aug 26, 2024
13 checks passed
@teunbrand teunbrand deleted the tibble_to_suggests branch August 26, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants