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
|
I'll need to some help reviewing the test scripts I wrote for this change. I haven't written any myself to this degree. I'm also still seeing some errors when I still build_site and document() the package but maybe I implemented the changes incorrectly? @bundfussr @bms63 |
Pushing again and confirmed check_type argument is in derive_var_obs_number in derive_joined.R scripts
|
You need to fix this conflict. You might need to accept the updated snapshot |
…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
Hey @bms63 accepted the snapshots and passed the dev tools::check() on my end but it fails when pushed. I noticed in the failure output that the snapshots I accepted didn't seem to go through. Did I miss something? |
2. Made suggested fixes to derive_param_tte script.
…ions. Directly called rlang::try_fetch in derive_param_tte script.
|
| warning = function(cnd) { | ||
| # Handle warnings | ||
| if (grepl("duplicate records", conditionMessage(cnd))) { | ||
| cli::cli_warn(c( | ||
| "Dataset '{.val {sources[[i]]$dataset_name}}' contains duplicate records.", | ||
| "i Duplicates were identified based on variables: | ||
| {.val {paste(c(subject_keys, by_vars, source_date_var), collapse = ', ')}}." | ||
| )) | ||
| } | ||
| source_dataset %>% | ||
| filter_if(sources[[i]]$filter) %>% | ||
| arrange(!!!sources[[i]]$order) # Return filtered dataset even if a warning occurred | ||
| }, | ||
| error = function(err) { | ||
| cli::cli_abort(c( | ||
| "Duplicate records detected during processing.", | ||
| "x Duplicate records were found in dataset {.val {sources[[i]]$dataset_name}}.", | ||
| "i The duplicates were identified based on the following variables: | ||
| {.val {paste(c(subject_keys, by_vars, source_date_var), collapse = ', ')}}.", | ||
| "i Consider reviewing your `by_vars` or `order` argument to ensure uniqueness." | ||
| )) | ||
| }, | ||
| message = function(msg) { | ||
| cli::cli_inform(c( | ||
| "Processing dataset '{.val {sources[[i]]$dataset_name}}'...", | ||
| "i Filter and order criteria: {.val {paste(c(subject_keys, by_vars, | ||
| sources[[i]]$order), collapse = ', ')}}." | ||
| )) | ||
| } |
There was a problem hiding this comment.
Once #2592 is merged this could be simplified:
duplicate_records = function(cnd) {
cnd_funs <- list(message = cli_inform, warning = cli_warn, error = cli_abort)
cnd_funs[[check_type]](
paste(
"Dataset {.val {sources[[i]]$dataset_name}} contains duplicate records with respect to",
"{.var {cnd$by_vars}}"
),
class = class(cnd))
cnd_muffle(cnd)
zap()
}
|
@ProfessorP-beep how's it going with the review updates for this one? I only ask as it impacts updates for our internal admiralroche package that I am assigned to. Hoping this one could merge before the holidays and then I could take care of mine early Jan. |
Sorry, making these changes this afternoon. |
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
| @@ -61,7 +61,7 @@ | |||
| i Following names were provided by `source_datasets`: "adsl" | |||
| i But, `censor_conditions[[1]]$dataset_name = adls` | |||
|
|
|||
| # list_tte_source_objects Test 14: error is issued if package does not exist | |||
| # list_tte_source_objects Test 16: error is issued if package does not exist | |||
There was a problem hiding this comment.
I think this should be 17
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()