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

pillar::num type vector is not properly treated by base::sum (with respect to na.rm=TRUE) #659

Closed
nirguk opened this issue Jun 12, 2024 · 1 comment · Fixed by #660
Closed

Comments

@nirguk
Copy link

nirguk commented Jun 12, 2024

user bvb_lc on the community forum highlighted an issue they encountered, a minimal reprex of it as follows

sum(pillar::num(c(1,NA)),na.rm=TRUE)

unfortunately the above results in

<pillar_num[1]>
[1] NA

rather than the desired

<pillar_num[1]>
[1] 1

link to source : https://forum.posit.co/t/na-rm-true-seems-to-not-work-for-a-tibble-with-the-datatype-of-pillar-num/187988

@krlmlr
Copy link
Member

krlmlr commented Jun 13, 2024

Thanks, very interesting:

sum(vctrs::new_vctr(c(1, NA), class = c("pillar_num", "pillar_vctr")), na.rm = TRUE)
#> <pillar_num[1]>
#> [1] 1
requireNamespace("pillar", quietly = TRUE)
sum(vctrs::new_vctr(c(1, NA), class = c("pillar_num", "pillar_vctr")), na.rm = TRUE)
#> <NULL[1]>
#> [1] NA

Created on 2024-06-13 with reprex v2.1.0

Looks like we need to pass along the ellipsis to vec_arith_base() :

pillar:::vec_arith.pillar_num.default
#> function (op, x, y, ...) 
#> {
#>     "!!!!DEBUG vec_arith.pillar_num.default(`v(op)`)"
#>     out <- vec_arith_base(op, vec_proxy(x), vec_proxy(y))
#>     if (inherits(x, "pillar_num")) {
#>         vec_restore(out, x)
#>     }
#>     else {
#>         vec_restore(out, y)
#>     }
#> }
#> <bytecode: 0x11f0c9ba0>
#> <environment: namespace:pillar>

Created on 2024-06-13 with reprex v2.1.0

Happy to review a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants