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

tm_g_gh_lineplot will always contraint data using "AVISTICD", "BASE2" #121

Open
clarkliming opened this issue Feb 16, 2022 · 6 comments
Open
Assignees
Labels
bug Something isn't working core

Comments

@clarkliming
Copy link

Summary

tm_g_gh_lineplot will always validate if there are AVISITCD, BASE, and BASE2 variables (through constr_anl_chunks). if it is really needed, information about this should be added in the description/introduction part; if it is not, we should pass some argument to make the constraint process work

R session info

R version 3.6.3 (2020-02-29)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Red Hat Enterprise Linux

Matrix products: default
BLAS/LAPACK: /usr/lib64/libopenblas-r0.3.3.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 utils     datasets  methods   base     

other attached packages:
 [1] tiramisu_0.2.0              sessioninfo_1.1.1          
 [3] teal.devel_0.2.11           purrr_0.3.4                
 [5] tidyr_1.0.2                 forcats_0.5.0              
 [7] shinythemes_1.1.2           teal.goshawk_0.1.9         
 [9] goshawk_0.1.9               teal.modules.clinical_0.8.9
[11] tern_0.7.4                  optimx_2020-4.2            
[13] rtables_0.3.8               teal.modules.general_0.2.10
[15] magrittr_1.5                ggmosaic_0.3.3             
[17] teal.osprey_0.1.10          teal_0.9.5                 
[19] shiny_1.4.0.2               osprey_0.1.10              
[21] ggplot2_3.3.0               dplyr_1.0.0                
[23] data.table_1.12.8           random.cdisc.data_0.3.11   
[25] reticulate_1.15            

loaded via a namespace (and not attached):
 [1] nlme_3.1-148        httr_1.4.1          numDeriv_2016.8-1.1
 [4] tools_3.6.3         dfoptim_2018.2-1    backports_1.1.6    
 [7] sparkline_2.0       DT_0.13             R6_2.4.1           
[10] lazyeval_0.2.2      colorspace_1.4-1    withr_2.2.0        
[13] tidyselect_1.1.0    gridExtra_2.3       emmeans_1.4.6      
[16] curl_4.3            compiler_3.6.3      cli_2.0.2          
[19] mcr_1.2.1           expm_0.999-4        shinyjs_1.1        
[22] plotly_4.9.2.1      sandwich_2.5-1      colourpicker_1.0   
[25] labeling_0.3        scales_1.1.0        mvtnorm_1.1-0      
[28] stringr_1.4.0       digest_0.6.25       foreign_0.8-76     
[31] minqa_1.2.4         rio_0.5.16          pkgconfig_2.0.3    
[34] htmltools_0.4.0     lme4_1.1-23         styler_1.3.2       
[37] fastmap_1.0.1       htmlwidgets_1.5.1   rlang_0.4.6        
[40] readxl_1.3.1        generics_0.0.2      zoo_1.8-7          
[43] jsonlite_1.6.1      googlesheets4_0.2.0 zip_2.0.4          
[46] car_3.0-7           Matrix_1.2-18       fansi_0.4.1        
[49] Rcpp_1.0.4.6        DescTools_0.99.34   munsell_0.5.0      
[52] abind_1.4-5         lifecycle_0.2.0     stringi_1.4.6      
[55] multcomp_1.4-13     yaml_2.2.1          carData_3.0-3      
[58] MASS_7.3-51.6       grid_3.6.3          parallel_3.6.3     
[61] promises_1.1.0      ggrepel_0.8.2       crayon_1.3.4       
[64] miniUI_0.1.1.1      lattice_0.20-41     haven_2.2.0        
[67] cowplot_1.0.0       splines_3.6.3       hms_0.5.3          
[70] pillar_1.4.3        boot_1.3-25         estimability_1.3   
[73] codetools_0.2-16    glue_1.4.0          vctrs_0.3.1        
[76] nloptr_1.2.2.1      httpuv_1.5.2        testthat_2.3.2     
[79] cellranger_1.1.0    gtable_0.3.0        assertthat_0.2.1   
[82] utils.nest_0.2.9    openxlsx_4.1.4      mime_0.9           
[85] xtable_1.8-4        broom_0.5.6         coda_0.19-3        
[88] later_1.0.0         googledrive_1.0.0   survival_3.2-3     
[91] viridisLite_0.3.0   tibble_3.0.1        lmerTest_3.1-2     
[94] shinyWidgets_0.5.1  statmod_1.4.34      TH.data_1.0-10     
[97] ellipsis_0.3.0   

OS / Environment

  • OS: Red Hat Enterprise Linux Server 7.9
@clarkliming
Copy link
Author

in addition, this program assume "AVALU" and LBSTRESC exists; this should be either documented or updated to make it robust. I was preparing a mean plot for adqs, where the dataset structure is quite similar, but went into errors which is not well documented, like AVALU not found, LBSTRESC not found;

@Polkas
Copy link
Contributor

Polkas commented Apr 19, 2022

@npaszty can you let us know how you feel about this item. Thanks

@npaszty
Copy link
Contributor

npaszty commented Apr 19, 2022

@clarkliming @Polkas

yes, there are expectations that the variables exist since this package was built for biomarker scientists to visualize assays that fall into the same domain structure as LB and ADLB. Given the context I disagree with the "bug" label. with that said there are a number of design changes that would render the functions more generic and some design changes that would ease some of the requirements.

I don't see an ADQS specification in the Roche ADaM 1.1 dictionary so not sure what the baseline cross study structure would be and therefore what to design to be more generic for all permutations of longitudinal visualization.

I could see that for ADQS AVALU is not relevant. so could be set to NA but that would show up in the axis label as "(NA)" or if set to missing string then "( )" or if set to "?" then "(?)" or etc. Neither of which is super great but in my estimation there is a user solution here to pick something.

Not having LBSTRESC in an ADQS data set is certainly expected since it would be QSSTRESC instead. So could rename that to LBSTRESC but that would appear odd and interpreted as out of place in the app unless maybe documented in the "User Guide" tab.

AVISITCD is expected and is created from AVIST so that's not an issue. the rationale is that many AVISIT values are extraordinarily long and so we created a generic approach to be abbreviate without loosing any visit information. users can simply do AVISITCD <- AVISIT and AVISITCDN <- AVISITN (or something like that) if they don't want to create the abbreviations.

Not having BASE is odd to me since that column contains the baseline value and used to calculate change from baseline and %change from baseline which is not alien to the questionnaire data concept certainly at the subscale and scale scores records.

BASE2 is not a great design but similarly to BASE is used to calculate change from screening if need. if not them it's simple just set to NA.

In conclusion goshawk has some restrictions due to its provenance requirements and with that some requirements called out here are documented in the sample app code.

@mhallal1
Copy link
Collaborator

@npaszty so what is to be done here based on your previous comment?

@kpagacz
Copy link
Contributor

kpagacz commented May 19, 2022

@npaszty is it correct to say that your recommended approach here is to add the description of the requirements to the documentation of this module and don't change the current behaviour?

@denisovan31415 denisovan31415 self-assigned this May 19, 2022
@npaszty
Copy link
Contributor

npaszty commented May 19, 2022

@mhallal1 @kpagacz

i think it would be helpful to enhance the documentation to further call out expectations and requirements. i included quite a few comments in the sample app code already but trouble is people tend not to read details.

with the changes to sample app documentation not sure the best place to put this. clarkliming suggests updating documentation or adding function argument. former is easier.

but this issue really highlights bigger question about goshawk design.

  • years ago wen discussing with Adrian he suggested scrubbing the code to remove the hard dependencies of having variable names in the code vs. resolving parameters from input$ etc.
  • also equally long ago user feedback was that introducing BASE2 as change from baseline isnt conventional to ADaM structure.
  • more recently there was another request to enhance the sample app documentation and to also include a ddl goshawk example.

i'm happy to address the documentation updates. would that still be in agile-r or given teal gallery somewhere else?
with opening an issue in appropriate documentation repo i think we could close this when that other issue is closed.
or maybe a vignette in the package?
what do you guys think?

gogonzo added a commit that referenced this issue Aug 9, 2023
part of #121 

added the required columns in the roxgyen docs.

And also similarly mentioned the required columns in the gosahawk app of
teal.gallery:

https://code.roche.com/nest/r-packages/teal.gallery/-/merge_requests/18

---------

Co-authored-by: Konrad Pagacz <[email protected]>
Co-authored-by: Dawid Kałędkowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core
Projects
None yet
Development

No branches or pull requests

7 participants