atorus-research / Tplyr

https://atorus-research.github.io/Tplyr/
Other
95 stars 16 forks source link

`where` in `group_count()` generates error #106

Closed tdwils closed 1 year ago

tdwils commented 1 year ago

Prerequisites

Background: For tables that require more than one section of group counts, a separate filter needs to be applied for each group count. For example, an adverse events summary table shows the number of AEs that are related (AEREL != "NONE"), serious (AESER == "Y"), etc. Each group_count() needs its own filter.

Description

After updating {Tplyr} from v0.4.4 to v1.0.1, the behavior of the where option seems to have changed. When the target dataset is different from the population dataset, and the where condition is placed in the group_count() function, an error is now produced by get_numeric_data() and build().

If the target and population datasets are the same, or if the where is placed in the tplyr_table() function, then it runs without error.

Steps to Reproduce

Reproducible example: target dataset = adae population dataset = adsl filter for this layer: AEREL != "NONE"

library(dplyr)
library(Tplyr)
library(admiral)
library(admiral.test)

data("admiral_adsl")
data("admiral_ae")

adsl <- admiral_adsl
# Basic ADAE dataset
adae <- admiral_ae %>% left_join(adsl, by = c("STUDYID", "USUBJID"))

t <- tplyr_table(adae, TRT01A) %>% 
  set_pop_data(adsl) %>% 
  set_pop_treat_var(TRT01A) %>% 
  set_pop_where(TRUE) %>%
  add_layer(
    group_count(AEDECOD, where = AEREL != "NONE") %>% 
      set_distinct_by(USUBJID)
  )

num_data <- get_numeric_data(t)
# Error in `filter()`:
#   ! Problem while computing `..1 = AEREL != "NONE"`.
# Caused by error:
#   ! object 'AEREL' not found

t_df <- build(t) 
# Error in `filter()`:
#   ! Problem while computing `..1 = AEREL != "NONE"`.
# Caused by error:
#   ! object 'AEREL' not found

Expected behavior:

get_numeric_data(t) and build(t) run without error.

Actual behavior:

# Error in `filter()`:
#   ! Problem while computing `..1 = AEREL != "NONE"`.
# Caused by error:
#   ! object 'AEREL' not found

Versions

R version 4.1.2 (2021-11-01)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19044)

Matrix products: default

locale:
[1] LC_COLLATE=English_United States.1252  LC_CTYPE=English_United States.1252   
[3] LC_MONETARY=English_United States.1252 LC_NUMERIC=C                          
[5] LC_TIME=English_United States.1252    

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

other attached packages:
[1] admiral.test_0.2.0 admiral_0.8.3      Tplyr_1.0.1        dplyr_1.0.8       

loaded via a namespace (and not attached):
 [1] rstudioapi_0.13  magrittr_2.0.2   hms_1.1.1        insight_0.15.0   tidyselect_1.1.1
 [6] R6_2.5.1         rlang_1.0.1      fansi_1.0.2      stringr_1.4.0    tools_4.1.2     
[11] utf8_1.2.2       cli_3.1.1        ellipsis_0.3.2   assertthat_0.2.1 tibble_3.1.6    
[16] lifecycle_1.0.1  crayon_1.5.0     purrr_0.3.4      tidyr_1.2.0      vctrs_0.3.8     
[21] sjlabelled_1.1.8 glue_1.6.1       haven_2.4.3      admiraldev_0.1.0 stringi_1.7.6   
[26] compiler_4.1.2   pillar_1.7.0     forcats_0.5.1    generics_0.1.2   lubridate_1.8.0 
[31] pkgconfig_2.0.3

Let me know if you need any other information. Thank you!

AARON-CLARK commented 1 year ago

+1

Agreed. This error has been a roadblock for our team @ Biogen as of {Tplyr} v1.0.1. In the meantime, we are requiring our users to use v0.4.4.

mstackhouse commented 1 year ago

Thanks for the report! I've verified this and we're looking into it!

mstackhouse commented 1 year ago

@elimillera error comes from here https://github.com/atorus-research/Tplyr/blob/d84e0653120766991fbefea4463c6500fb25b8ac/R/count.R#L670

The issue is that denom_where is AEREL != "None", so the layer level where is overwriting the population where - which given the pop data, that shouldn't be the case.

mstackhouse commented 1 year ago

Also @tdwils kudos on a really well written bug report!

elimillera commented 1 year ago

Thanks @tdwils and @AARON-CLARK. The branch 'bug-fix/issue-106' should have this resolved if you are able to give it a check.

tdwils commented 1 year ago

@elimillera Thank you for the quick response! I have confirmed that this issue has been resolved.

Do you have an estimate for when this would be pushed to CRAN?

mstackhouse commented 1 year ago

@tdwils we'll definitely push it up by early next week - I have some comments to Eli to look through but we'll get it pushed back up ASAP.

mstackhouse commented 1 year ago

@tdwils and @AARON-CLARK On it's way to CRAN!

tdwils commented 1 year ago

Thank you!