r-lib / pillar

Format columns with colour
https://pillar.r-lib.org/
Other
178 stars 37 forks source link

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

Closed nirguk closed 5 months ago

nirguk commented 5 months ago

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 commented 5 months ago

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.