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

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

Open
alexiswl opened this issue Mar 31, 2022 · 5 comments
Open

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

alexiswl opened this issue Mar 31, 2022 · 5 comments

Comments

@alexiswl
Copy link

Hello,

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

> library(rapportools)
Attaching package:rapportoolsThe following objects are masked frompackage:stats:

    IQR, median, sd, var

The following objects are masked frompackage: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
Copy link
Member

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 🤔

I suppose that's due to 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
Copy link
Author

alexiswl commented Mar 31, 2022

I suppose that's due to 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
Copy link
Author

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.

alexiswl added a commit to umccr/RNAsum that referenced this issue Mar 31, 2022
* Waiting for Rapporter/rapportools#4 to be resolved

* Use base:: notation for all sum installs
alexiswl added a commit to umccr/RNAsum that referenced this issue Apr 26, 2022
* Waiting for Rapporter/rapportools#4 to be resolved

* Use base:: notation for all sum installs
alexiswl added a commit to umccr/RNAsum that referenced this issue Apr 26, 2022
* Waiting for Rapporter/rapportools#4 to be resolved

* Use base:: notation for all sum installs
alexiswl added a commit to umccr/RNAsum that referenced this issue Apr 27, 2022
* Waiting for Rapporter/rapportools#4 to be resolved

* Use base:: notation for all sum installs
@audiracmichelle
Copy link

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
Copy link
Author

@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.

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

No branches or pull requests

3 participants