Closes #88 Use pharmaversesdtm to host admiralpeds test data #115
Conversation
zdz2101
changed the title
|
zdz2101
requested review from
kaz462,
manciniedoardo,
Fanny-Gautier and
rossfarrugia
|
Do we hold off on this until first release of admiralpeds? |
Yes, it should be aligned with this update: pharmaverse/admiralpeds#40. I'm just catching up from last week and looks like we had more review comments for admiralpeds and the team are working through them still so no CRAN submission as of yet... |
|
@zdz2101 please let us know if you think this one is now ready for review given we've released admiralpeds to CRAN, and me and @Fanny-Gautier could take a look |
this is ready for review, the one on |
rossfarrugia
changed the title
|
@kaz462 @manciniedoardo @Fanny-Gautier appreciate if you could give this PR a sanity check too. |
Co-authored-by: Ross Farrugia <[email protected]>
Co-authored-by: Ross Farrugia <[email protected]>
| VSBLFL = if_else(VISITNUM == 2, "Y", NA_character_), | ||
| VISITDY = NA_integer_, | ||
| VSDY = NA_integer_, | ||
| VSEVAL = NA_character_, | ||
| EPOCH = "Epoch" |
There was a problem hiding this comment.
Is it intentional to leave those columns blank (including VSBLFL), should we just remove this part and carry values over from the original VS data?
There was a problem hiding this comment.
By the way, should VSBLFL be replaced by VSLBOXFL? In the SDTMIG it is stated that VSBLFL is kept only for backward compatibility. See also "4.5.9 Baseline Values" in the SDTMIG.
There was a problem hiding this comment.
@bundfussr want to make a separate issue on that one in this repo? as it would impact all VS (and any BDS) datasets, not just this peds one
data-raw/dm_peds.R
Outdated
| library(admiral) | ||
|
|
||
| # Read input test data from pharmaversesdtm ---- | ||
| data("dm") |
There was a problem hiding this comment.
I could see in another PR that the use of data() should now be avoided. Is it applicable here ?
data-raw/vs_peds.R
Outdated
| library(admiral) | ||
|
|
||
| # Read input test data from pharmaversesdtm ---- | ||
| data("vs") |
There was a problem hiding this comment.
I could see in another PR that the use of data() should now be avoided. Is it applicable here ?
|
@rossfarrugia @Fanny-Gautier This can be reviewed once again |
|
@kaz462 @manciniedoardo @rossfarrugia can we merge this? I'm getting a "restricts merging to authorized users" |
Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.
Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the
develbranch until you have checked off each task.styler::style_file()to style R and Rmd filesdevtools::document()so all.Rdfiles in themanfolder and theNAMESPACEfile in the project root are updated appropriatelyNEWS.mdif the changes pertain to a user-facing function (i.e. it has an@exporttag) or documentation aimed at users (rather than developers)pkgdown::build_site()and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.lintr::lint_package()R CMD checklocally and address all errors and warnings -devtools::check()