Closes #2481 bug the result of derive param tte depends on the sort order of the input #2569
Conversation
…ignal_duplicate_records to derive_param_tte. Still troubleshooting the test-derive_param_tte script. Failed tests have a "Required variable `AEDECOD` is missing in `dataset`" error.
…9, 15, and 16 in test-derive_param_tte
…_tte to fix missing data error
This comment was marked as resolved.
This comment was marked as resolved.
Pushing again and confirmed check_type argument is in derive_var_obs_number in derive_joined.R scripts
This comment was marked as resolved.
This comment was marked as resolved.
…om test-derive_param_tte as it was redundant, and ran pharmaverse4devs format test script addin to format testest-derive_param_tte.
…nds-on-the-sort-order-of-the-input
…le dataset_adsl and source_datasets by combining them with bind_rows before to address error of AEDECOD missing from the dataset when just calling dataset_adsl. This starts on line 381 of derive_param_tte.R
…sort-order-of-the-input' of https://github.com/pharmaverse/admiral into 2481-bug-the-result-of-derive_param_tte-depends-on-the-sort-order-of-the-input
…ess failed runs in Test 16 of test-derive_param_tte. removed signal_duplicate_records() from within derive_param_tte Still need to troubleshoot errors in test script.
…ote Test 15 and 16 on test-derive_param_tte to deal with update to duplicate warnings within tryCatch and not directly by signal_duplicate_records inside derive_param_tte function. Accepted snapshots from devtools::check
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2. Made suggested fixes to derive_param_tte script.
…ions. Directly called rlang::try_fetch in derive_param_tte script.
|
This comment was marked as outdated.
This comment was marked as outdated.
R/derive_param_tte.R
Outdated
| #' @param order Sort order | ||
| #' | ||
| #' If the argument is set to a non-null value, for each by group the first or | ||
| #' last observation |
There was a problem hiding this comment.
@ProfessorP-beep from a user perspective i find this argument description confusing. I'm unclear whether i need to use exprs() like for set_values_to explained above and i don't really understand the description here. I thought it would be worded more something like:
An optional named list returned by exprs() defining additional variables that the input dataset is sorted on after date.
There was a problem hiding this comment.
@bundfussr any thoughts on this suggestion? just in case i'm missing anything.
There was a problem hiding this comment.
@rossfarrugia , I agree that the description should be updated. I would move the technical details to "Permitted Values". For example
Additional variables that the source dataset is sorted on after
date.Permitted Values: list of variables created by
exprs()e.g.exprs(ASEQ)
rossfarrugia
changed the title
There was a problem hiding this comment.
@ProfessorP-beep thanks for updating for my comments! i spotted a few minor things in the updates
R/derive_param_tte.R
Outdated
| #' ~USUBJID, ~AESTDTC, ~AESEQ, ~AEDECOD, | ||
| #' "01", "2021-01-03T10:56", 1, "Flu", | ||
| #' "01", "2021-03-04", 2, "Cough", | ||
| #' "01", "2021", 3, "Flu" | ||
| #' ~USUBJID, ~AESTDTC, ~AESEQ, ~AEDECOD, | ||
| #' "01", "2021-01-03T10:56", 1, "Flu", | ||
| #' "01", "2021-03-04", 2, "Cough", | ||
| #' "01", "2021-", 3, "Flu" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Stefan Bundfuss <[email protected]>
…nds-on-the-sort-order-of-the-input
|
Hi @ProfessorP-beep are we on track with updates to be completed by EOD Friday. We need to give @bundfussr some time for final review. Many thanks for tackling this update!! |
bms63
changed the title
…sort-order-of-the-input' of https://github.com/pharmaverse/admiral into 2481-bug-the-result-of-derive_param_tte-depends-on-the-sort-order-of-the-input
Yea, we should be good. I'm just going to fix the errors from this last push. |
There was a problem hiding this comment.
derive_var_obs_number() needs to be updated such that check_type = "message" is accepted.
Otherwise,
derive_param_tte(
dataset_adsl = adsl,
start_date = TRTSDT,
event_conditions = list(ttae),
censor_conditions = list(eot),
source_datasets = list(adsl = adsl, ae = ae),
set_values_to = exprs(PARAMCD = "TTAE"),
check_type = "message"
)
fails.
|
@bms63 sorry that request was an accident. I meant to request review from @bundfussr |
all good! i made a suggestion in the news! Can you hide comments where the issue was addressed? Also good just to hide all the comments discussion so we can see if anything needs to finalized. |
Co-authored-by: Ben Straub <[email protected]>
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
mainbranch until you have checked off each task.styler::style_file()to style R and Rmd filesinst/cheatsheet/admiral_cheatsheet.pptxand re-upload a PDF and a PNG version of it to the same folder. (The PNG version can be created by taking a screenshot of the PDF version.)devtools::document()so all.Rdfiles in themanfolder and theNAMESPACEfile in the project root are updated appropriatelyNEWS.mdunder the header# admiral (development version)if the changes pertain to a user-facing function (i.e. it has an@exporttag) or documentation aimed at users (rather than developers). A Developer Notes section is available inNEWS.mdfor tracking developer-facing issues.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()