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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7bd767c
Call vec_restore() on proxy objects only
krlmlr Apr 18, 2021
e8952bc
Merge branch 'docs-restore' into b-restore
krlmlr Apr 18, 2021
bb6356a
Merge branch 'master' into b-restore
krlmlr Jun 9, 2021
5037bc4
Take proxy before operating, restore on original
krlmlr Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ importFrom(vctrs,vec_data)
importFrom(vctrs,vec_is)
importFrom(vctrs,vec_math)
importFrom(vctrs,vec_math_base)
importFrom(vctrs,vec_proxy)
importFrom(vctrs,vec_proxy_compare)
importFrom(vctrs,vec_proxy_order)
importFrom(vctrs,vec_ptype2)
Expand Down
2 changes: 1 addition & 1 deletion R/char.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}
8 changes: 4 additions & 4 deletions R/num.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


if (inherits(x, "pillar_num")) {
vec_restore(out, x)
Expand All @@ -199,7 +199,7 @@ vec_arith.pillar_num.MISSING <- function(op, x, y, ...) {
"!!!!DEBUG vec_arith.pillar_num.MISSING(`v(op)`)"
stopifnot(is.numeric(x))
# FIXME
out <- vec_arith_base(op, 0, x)
out <- vec_arith_base(op, 0, vec_proxy(x))

vec_restore(out, x)
}
Expand All @@ -214,7 +214,7 @@ vec_math.pillar_num <- function(op, x, ...) {
"!!!!DEBUG vec_math(`v(op)`)"

stopifnot(is.numeric(x))
out <- vec_math_base(op, x)
out <- vec_math_base(op, vec_proxy(x))

if (is.numeric(out)) {
out <- vec_restore(out, x)
Expand Down Expand Up @@ -358,7 +358,7 @@ vec_cast.double.pillar_num <- function(x, to, ...) {
}
#' @export
vec_cast.pillar_num.double <- function(x, to, ...) {
vec_restore(x, to)
vec_restore(vec_proxy(x), to)
}
#' @export
vec_cast.pillar_num.integer <- function(x, to, ...) {
Expand Down
1 change: 1 addition & 0 deletions R/zzz.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#' @importFrom vctrs vec_is
#' @importFrom vctrs vec_math
#' @importFrom vctrs vec_math_base
#' @importFrom vctrs vec_proxy
#' @importFrom vctrs vec_proxy_compare
#' @importFrom vctrs vec_proxy_order
#' @importFrom vctrs vec_ptype_abbr
Expand Down