Open clarkliming opened 2 years ago
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;
@npaszty can you let us know how you feel about this item. Thanks
@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.
@npaszty so what is to be done here based on your previous comment?
@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?
@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.
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?
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
OS / Environment