Rework vec_ptype() into a generic for S3 types - #1322
Merged
DavisVaughan merged 7 commits intoMar 11, 2021
Conversation
DavisVaughan
force-pushed
the
feature/vec-ptype-generic
branch
from
February 25, 2021 13:24
b149397 to
14b3c2c
Compare
Member
Author
|
Possible benefits for integer64, which could be added in a followup PR Compare against #1323 (comment)
library(vctrs, warn.conflicts = FALSE)
vctrs_empty_integer64 <- NULL
vec_ptype.integer64 <- function(x, ...) {
if (is.null(vctrs_empty_integer64)) {
vctrs_empty_integer64 <<- bit64::integer64()
}
out <- vctrs_empty_integer64
dim <- dim(x)
if (is.null(dim)) {
return(out)
}
if (length(dim) == 0L) {
abort("Corrupt `dim` attribute. `dim` must have length >=1.")
}
dim[[1]] <- 0L
dim(out) <- dim
out
}
library(dplyr, warn.conflicts = FALSE)
suppressPackageStartupMessages(library(bit64))
#> Warning: package 'bit64' was built under R version 4.0.2
#> Warning: package 'bit' was built under R version 4.0.2
df <- tibble(x = 1, x64 = integer64(1), xstr = "1", y = sample(10000, 1000000, replace = T)) %>% group_by(y)
first_i64 <- group_map(df, ~.x$x64[1])
first_int <- group_map(df, ~.x$x[1])
type_common <- function(x) vec_ptype_common(!!!x)
vec_c2 <- function(x) vec_c(!!!x)
bench::mark(
type_common(first_int),
vec_c2(first_int),
type_common(first_i64),
vec_c2(first_i64),
check = FALSE,
iterations = 20
)
#> Warning: Some expressions had a GC in every iteration; so filtering is disabled.
#> # A tibble: 4 x 6
#> expression min median `itr/sec` mem_alloc `gc/sec`
#> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl>
#> 1 type_common(first_int) 606.1µs 680.31µs 1402. 4.02KB 0
#> 2 vec_c2(first_int) 1.88ms 2.27ms 458. 195.45KB 0
#> 3 type_common(first_i64) 175.62ms 201.31ms 4.88 42.09KB 21.0
#> 4 vec_c2(first_i64) 1.19s 1.26s 0.795 764.06MB 12.2 |
lionel-
approved these changes
Mar 10, 2021
| #' See [internal-faq-ptype2-identity] for more information about | ||
| #' identity values. | ||
| #' | ||
| #' For performance, when developing a new S3 class you might want to override |
Member
There was a problem hiding this comment.
I think the main consequence of the "static prototype" argument is that names are no longer part of the prototype. This is something that matches my intuition and longer-term plans: #1020
For the patch release we could merge this to unblock Davis (performance of clock classes). Then at the next non-patch release we'll unname inputs before slicing in the default vec_ptype() method. Does that sound good @hadley?
DavisVaughan
force-pushed
the
feature/vec-ptype-generic
branch
from
March 11, 2021 14:56
abea16f to
b999f25
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Demonstrated benefits of a static vec-ptype:
(It is a bit oversimplified for year-month-day, but the performance improvements remains valid)
It would be up to the developer to ensure that their vec-ptype method returns a "valid" ptype. I think this is a reasonable requirement, since we essentially require that their vec-ptype2 and vec-cast methods are also valid.
Created on 2021-02-24 by the reprex package (v1.0.0)