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

treat vctrs record rcrd as a list #819

Closed
maxheld83 opened this issue Feb 24, 2021 · 2 comments
Closed

treat vctrs record rcrd as a list #819

maxheld83 opened this issue Feb 24, 2021 · 2 comments

Comments

@maxheld83
Copy link

maxheld83 commented Feb 24, 2021

A vctrs record type, while implemented as a list (of equal-lengths vectors) under the hood, is otherwise treated as "atomic" externally (loosely speaking).
My naive expectation had been that purrr::map() would treat it thus and loop over it's length, not over the list of its internal structure.

library(vctrs)

new_rational <- function(n = integer(), d = integer()) {
  # assertions from vignette dropped
  new_rcrd(list(n = n, d = d), class = "vctrs_rational")
}

format.vctrs_rational <- function(x, ...) {
  n <- field(x, "n")
  d <- field(x, "d")

  out <- paste0(n, "/", d)
  out[is.na(n) | is.na(d)] <- NA

  out
}

x <- new_rational(c(1, 1, 1), c(2, 2, 2))
# vctrs API sugggests a record, externally viewed, is NOT a list
vctrs::vec_is_list(x)
#> [1] FALSE
length(x)
#> [1] 3
# base R, oddly, agrees
lapply(x, str)
#>  vctrs_rt [1:1] 1/2
#>  vctrs_rt [1:1] 1/2
#>  vctrs_rt [1:1] 1/2
#> [[1]]
#> <vctrs_rational[1]>
#> [1] 1/2
#> 
#> [[2]]
#> <vctrs_rational[1]>
#> [1] 1/2
#> 
#> [[3]]
#> <vctrs_rational[1]>
#> [1] 1/2
# but purrr treats a vctrs record as a list of *fields*
purrr::map(x, str)
#>  vctrs_rt [1:1] 1/2
#>  vctrs_rt [1:1] 1/2
#> $n
#> <vctrs_rational[1]>
#> [1] 1/2
#> 
#> $d
#> <vctrs_rational[1]>
#> [1] 1/2

Created on 2021-02-25 by the reprex package (v1.0.0)

My expectation had been that purrr::map() would not break the external appearance of records and that, had I needed to get into the fields, I would have had to do that myself:

map(x, function(i) {
  paste0(vctrs::field(i, "n"), vctrs::field(i, "d"))
})

Ps.: I've read many of the issues and lengthy discussions on map() and vctrs, and I hope this isn't a duplicate.
I'm guessing r-lib/vctrs#495 is the closest one, though that's about implementing vctrs::vec_map().

@hadley
Copy link
Member

hadley commented Aug 23, 2022

It's not 100% clear what we should do here — is it better for map() to be consistent with base R and use length() + [[ or be consistent with vctrs and use vec_size() and vec_slice().

However, if we were to change from the existing behaviour to be compatible with vctrs, map(df) would change from mapping over columns to mapping over rows, likely breaking a lot of code. That means if we did implement this, it would need to be in a new function.

@hadley
Copy link
Member

hadley commented Aug 27, 2022

See #683 for a probable approach.

@hadley hadley closed this as completed Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants