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

Can't cast <logical> to <vctrs_unspecified> in tibbles? #800

Closed
sebkopf opened this issue Feb 9, 2020 · 2 comments · Fixed by #816
Closed

Can't cast <logical> to <vctrs_unspecified> in tibbles? #800

sebkopf opened this issue Feb 9, 2020 · 2 comments · Fixed by #816

Comments

@sebkopf
Copy link

sebkopf commented Feb 9, 2020

The following cast errors but only for logical NA and only for tibbles (see example below). Maybe this is related to #205? Sorry, not familiar enough yet with vctrs, could not figure out why this fails.

vctrs::vec_rbind(
  dplyr::tibble(x = 1), 
  dplyr::tibble(x = 2, y = NA)
)
# Error: Can't cast `y` <logical> to `y` <vctrs_unspecified>.

If using e.g. an integer NA it works as expected. Same for logical NA in a base R data frame:

vctrs::vec_rbind(
    dplyr::tibble(x = 1), 
    dplyr::tibble(x = 2, y = NA_integer_)
)
## A tibble: 2 x 2
#      x     y
#  <dbl> <int>
#1     1    NA
#2     2    NA

vctrs::vec_rbind(
    data.frame(x = 1), 
    data.frame(x = 2, y = NA)
)
#   x  y
#1 1 NA
#2 2 NA
sebkopf added a commit to isoverse/isoreader that referenced this issue Feb 9, 2020
@DavisVaughan
Copy link
Member

DavisVaughan commented Feb 13, 2020

I think there are two issues here. I'll give this a shot tomorrow

The first is that if the C vec_type() function sees a tibble, it considers it an S3 object and gets the type with vec_slice(x, NULL). This actually isn't a completely foolproof way to get the type of a data frame. By slicing a column of all NA values, you end up with an empty logical column, when it should really be considered unspecified. The approach taken with bare data frames of mapping over each column and querying the type with vec_type() is the correct approach.

library(dplyr, warn.conflicts = FALSE)
library(vctrs)

# wrong
vec_ptype(tibble(x = 2, y = NA))
#> # A tibble: 0 x 2
#> # … with 2 variables: x <dbl>, y <lgl>

# correct
as_tibble(vec_ptype(data.frame(x = 2, y = NA)))
#> # A tibble: 0 x 2
#> # … with 2 variables: x <dbl>, y <???>

Once that is fixed, then these two should have the same results. This mimics what vec_ptype_common() does before it finalises the result with vec_ptype_finalise(). Before we finalise, I expect that we should have a common type of a tibble with a double column and an unspecified column.

library(dplyr, warn.conflicts = FALSE)
library(vctrs)

# vec_ptype_common(dplyr::tibble(x = 1), dplyr::tibble(x = 2, y = NA))
# but before the finalize is run
vec_ptype2(
  vec_ptype2(NULL, dplyr::tibble(x = 1)),
  dplyr::tibble(x = 2, y = NA)
)
#> # A tibble: 0 x 2
#> # … with 2 variables: x <dbl>, y <???>

# vec_ptype_common(dplyr::tibble(x = 2, y = NA), dplyr::tibble(x = 1))
# but before the finalize is run
vec_ptype2(
  vec_ptype2(NULL, dplyr::tibble(x = 2, y = NA)),
  dplyr::tibble(x = 1)
)
#> # A tibble: 0 x 2
#> # … with 2 variables: x <dbl>, y <lgl>

The second problem is with vec_ptype_finalise(). I think it needs to be recursive over data frames (In fact, it used to be). If we finalise the data frame above with the unspecified column, we still get a data frame with an unspecified column back. We should get a logical column there instead.

library(dplyr, warn.conflicts = FALSE)
library(vctrs)

internal_common_type <- vec_ptype2(
  vec_ptype2(NULL, dplyr::tibble(x = 1)),
  dplyr::tibble(x = 2, y = NA)
)

internal_common_type
#> # A tibble: 0 x 2
#> # … with 2 variables: x <dbl>, y <???>

vec_ptype_finalise(internal_common_type)
#> # A tibble: 0 x 2
#> # … with 2 variables: x <dbl>, y <???>

@lionel-
Copy link
Member

lionel- commented Feb 14, 2020

Agreed finalisation should be recursive over data frames.

sebkopf added a commit to isoverse/isoreader that referenced this issue Jul 5, 2020
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 a pull request may close this issue.

3 participants