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

Call vec_restore() on original objects, call vec_proxy() before operating #316

Merged
merged 4 commits into from
Jul 22, 2021

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Apr 18, 2021

.

Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

Restoration is towards the original input, not the proxy.

  • Take the proxy
  • Operate on proxy
  • Restore result to input

@krlmlr krlmlr requested a review from lionel- June 9, 2021 04:09
@krlmlr krlmlr changed the title Call vec_restore() on proxy objects only Call vec_restore() on original objects, call vec_proxy() before operating Jun 9, 2021
@@ -198,5 +198,5 @@ vec_cast.character.pillar_char <- function(x, to, ...) {
}
#' @export
vec_cast.pillar_char.character <- function(x, to, ...) {
vec_restore(x, to)
vec_restore(vec_proxy(x), to)
Copy link
Member

Choose a reason for hiding this comment

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

hmm actually in this case this doesn't make much of a difference since cast methods are not inherited. So you're guaranteed to get a bare character. Sorry for misleading you here.

@@ -185,7 +185,7 @@ vec_arith.pillar_num <- function(op, x, y, ...) {
vec_arith.pillar_num.default <- function(op, x, y, ...) {
"!!!!DEBUG vec_arith.pillar_num.default(`v(op)`)"
stopifnot(is.numeric(x), is.numeric(y))
out <- vec_arith_base(op, x, y)
out <- vec_arith_base(op, vec_proxy(x), vec_proxy(y))
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is quite right, but I don't have any better advice for generic vec_arith() at the moment. Since you check for is.numeric() above, it feels a bit unnecessary to take the proxy because you're restricting the inputs to have numeric storage.

Also conceptually there is no general guarantee that vec_proxy() is the adequate numeric storage for arithmetic operations. For instance the proxy might be a data frame (record type) and the numeric storage might be in a column of that data frame. For the moment though, we don't have anything better in vctrs.

@krlmlr krlmlr merged commit c5ca9c9 into master Jul 22, 2021
@krlmlr krlmlr deleted the b-restore branch July 22, 2021 03:08
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants