Rapporter / rapportools

Miscellaneous stats helper functions with sane defaults for reporting
8 stars 1 forks source link

sum with na.rm=TRUE adds another value?? #4

Open alexiswl opened 2 years ago

alexiswl commented 2 years ago

Hello,

This appears to be an issue introduced into rapportools v1.1

> library(rapportools)
Attaching package: ‘rapportools’

The following objects are masked from ‘package:stats’:

    IQR, median, sd, var

The following objects are masked from ‘package:base’:

    max, mean, min, range, sum

> my_list = c("c", "d", "e")

> sum(c("e" %in% my_list))
[1] 1

> sum(c("e" %in% my_list), na.rm=TRUE)
[1] 2

> sessionInfo()
R version 4.1.3 (2022-03-10)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 20.04.4 LTS

Matrix products: default
BLAS:   /opt/R/4/4.1.3/lib/R/lib/libRblas.so
LAPACK: /opt/R/4/4.1.3/lib/R/lib/libRlapack.so

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C
 [9] LC_ADDRESS=C               LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C

attached base packages:
[1] stats     graphics  grDevices datasets  utils     methods   base

other attached packages:
[1] rapportools_1.1 reprex_2.0.1

loaded via a namespace (and not attached):
 [1] Rcpp_1.0.8.3        digest_0.6.29       withr_2.5.0
 [4] MASS_7.3-56         plyr_1.8.7          magrittr_2.0.2
 [7] rlang_1.0.2         stringi_1.7.6       cli_3.2.0
[10] renv_0.15.4         reshape2_1.4.4      rstudioapi_0.13
[13] fs_1.5.2            tools_4.1.3         stringr_1.4.0
[16] pander_0.6.5        glue_1.6.2          compiler_4.1.3
[19] BiocManager_1.30.16 clipr_0.8.0
daroczig commented 2 years ago

Hm, this seems to be not passing na.rm = TRUE to base::sum as its na.rm arg, but instead including that in the addition :thinking:

I suppose that's due to https://github.com/Rapporter/rapportools/compare/v1.0...v1.1#diff-edee52205cb468aab8add6b6c383302050e965bacfedede0653218751c4387afL151-R151, but I will not have the time to do a throughout testing in the immediate future -- would appreciate any related help there.

alexiswl commented 2 years ago

I suppose that's due to https://github.com/Rapporter/rapportools/compare/v1.0...v1.1diff-edee52205cb468aab8add6b6c383302050e965bacfedede0653218751c4387afL151-R151, but I will not have the time to do a throughout testing in the immediate future -- would appreciate any related help there.

That link doesn't work for me but I assume you're referring to: https://github.com/Rapporter/rapportools/blob/v1.1/R/univar.R#L150-L151

Could explicity parse these arguments in for each wrapper of univar? so max, mean, min etc? and along with the stats functions that are masked, IQR, median, sd etc.

i.e

sum <- function(...)
    univar(..., fn = base::sum)

Becomes

sum <- function(..., na.rm=TRUE){
    rapportools:::univar(..., na.rm=na.rm, fn = base::sum)
}

This seems to work

> library(rapportools)
> my_list <- c("a", "b", "c")
> sum("a" %in% my_list, na.rm=TRUE)
[1] 2

> sum <- function(..., na.rm=TRUE){
       rapportools:::univar(..., na.rm=na.rm, fn = base::sum)
   }

> sum("a" %in% my_list)
[1] 1
alexiswl commented 2 years ago

I understand you might be pressed for time but it's a pretty serious bug given that these functions mask the base functions but behave quite differently without erroring and would kindly ask that this is prioritized. It took me quite a while to narrow down the issue.

audiracmichelle commented 1 year ago

I re-installed the package and the issue is still present. The bug made me waste at least two hours, I was desperately trying to find where I was introducing 1's.

alexiswl commented 1 year ago

@audiracmichelle my recommendation is either downgrade to 1.0 or don't use this package.

Would recommend renv for package management for your projects.

We've had to implement such to make sure our Rscripts work when using this package.