2481 bug the result of derive param tte depends on the sort order of the input #2569

ProfessorP-beep · 2024-11-18T16:45:14Z

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 main branch until you have checked off each task.

…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.

…ixes.

…9, 15, and 16 in test-derive_param_tte

… valid argument

…_tte to fix missing data error

ProfessorP-beep · 2024-11-18T16:47:56Z

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

bms63 · 2024-11-18T16:52:41Z

R/derive_joined.R

@@ -478,7 +478,7 @@ derive_vars_joined <- function(dataset,
    derive_var_obs_number(
      new_var = !!tmp_obs_nr,
      by_vars = by_vars_left,
-      check_type = "none"
+      "none"


How come this argument got dropped?

I am not sure. Let me add it back in. I did a fresh pull before working on this again but I likely made a mistake

Its still there on my end. I'll push this again.

Reviewing to see if I made any other mistakes.

Pushing again and confirmed check_type argument is in derive_var_obs_number in derive_joined.R scripts

bms63 · 2024-11-18T17:09:42Z

You need to fix this conflict. You might need to accept the updated snapshot

bundfussr · 2024-11-18T17:26:29Z

tests/testthat/test-derive_param_tte.R

-        PARAM = past("Time to First", AEDECOD, "Adverse Event"),
+        PARAM = paste("Time to First", AEDECOD, "Adverse Event"),


Please don't change this. The mistake is intentional to cause an error. Now the test is failing because there is no error anymore.

I see. I thought I had changed this by mistake when getting errors back. I'll change it back. Thanks.

bundfussr · 2024-11-18T17:30:32Z

R/derive_param_tte.R

+  # check for duplicates in event_data
+  signal_duplicate_records(
+    dataset = event_data,
+    by_vars = expr_c(by_vars, subject_keys),
+    cnd_type = check_type
+  )


This check will never fail because filter_date_sources() returns at most one records per by group and subject. That's the reason why the test "derive_param_tte detects duplicates when check_type = 'warning'" fails.

I think I get it. I just made a push, but let me take a look.

Okay, let me know if this was effective. The error I was getting was that AEDECOD was not being included in the signal_duplicate_records function. So I bound dataset_adsl and source_datasets together then ran the check within derive_param_tte like so:

#check for duplicates in dataset_adsl and source_datasets
combined_dataset <- bind_rows(dataset_adsl, !!!source_datasets)

signal_duplicate_records(
dataset = combined_dataset,
by_vars = expr_c(subject_keys, by_vars),
cnd_type = check_type
)

All the tests have passed now with some additional warnings. I have to run to some meetings and seminars today but am pushing now for review and can check back in the evening. On first glance not sure if the earlier tests should get a duplicate warning now as well.

tests/testthat/test-derive_param_tte.R

…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

tests/testthat/test-derive_param_tte.R

R/derive_param_tte.R

…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

ProfessorP-beep · 2024-11-25T17:54:14Z

You need to fix this conflict. You might need to accept the updated snapshot

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?

bms63

It looks like a different test is falling over

bundfussr · 2024-11-26T09:21:58Z

R/derive_param_tte.R

+            order = exprs(!!source_date_var),
+            by_vars = expr_c(subject_keys, by_vars),
+            mode = mode,
+            check_type = check_type


At the moment check_type = "message" is not accepted by filter_extreme(). Could you change this?

bundfussr · 2024-11-26T09:24:11Z

R/derive_param_tte.R

+          arrange(!!!sources[[i]]$order) %>% # Ensure order is applied
+          filter_extreme(
+            order = exprs(!!source_date_var),


The order specified for the tte_source object should be added to the order argument in the filter_extreme() call and it should be after !!source_date_var. This ensures that the records are ordered first by date and that all order variables are displayed in the message in case that there are duplicates.

bundfussr · 2024-11-26T09:41:43Z

R/derive_param_tte.R

 #' )
 filter_date_sources <- function(sources,
                                source_datasets,
                                by_vars,
                                create_datetime = FALSE,
                                subject_keys,
-                                mode) {
+                                mode,
+                                check_type = "none") {


The new argument should be added to the documentation.

bundfussr · 2024-11-26T09:45:00Z

R/derive_param_tte.R

+            conditionMessage(wrn)
+          ), call. = FALSE)
+        }
+        return(source_dataset)


The function should not return source_dataset but but the filtered source dataset.

bundfussr · 2024-11-26T09:52:19Z

R/derive_param_tte.R

+      },
+      warning = function(wrn) {
+        if (grepl("duplicate records", conditionMessage(wrn))) {
+          warning(sprintf(


cli_warn() should be used instead of warning().

The warning is duplicated, e.g., "Test 15" produces

Warning messages: 1: Dataset 'ae' contains duplicate records: Dataset contains duplicate records with respect to `STUDYID`, `USUBJID`, and `AESTDT` ℹ Run admiral::get_duplicates_dataset() to access the duplicate records 2: Dataset contains duplicate records with respect to `STUDYID`, `USUBJID`, and `ADT` ℹ Run admiral::get_duplicates_dataset() to access the duplicate records

Could you update such that we get

Warning message: Dataset 'ae' contains duplicate records with respect to `STUDYID`, `USUBJID`, and `AESTDT` ℹ Run admiral::get_duplicates_dataset() to access the duplicate records

?

bundfussr · 2024-11-26T10:02:19Z

R/derive_param_tte.R

+            check_type = check_type
+          )
+      },
+      warning = function(wrn) {


This catches warnings only. Errors and messages should be handled as well. (Maybe https://rlang.r-lib.org/reference/try_fetch.html is helpful.)

bundfussr · 2024-11-26T10:04:51Z

NEWS.md

+
+- `check_type = "warning"` default argument added to `derive_param_tte` with an
+  `arg_match` function within the function so the user can use a valid input of 
+  `error, message, warning, or none`. `signal_duplicate_records()` has also been
+  added to the function on lines 394 and 411 to check for uniqueness of records. (#2481)
+
+- `order()` function has been added to `event_source()` and `censor_source()` and
+  defaulted to `NULL` to allow sorting of input data. (#2481)
+


This should be moved up to the "development version" section because admiral 1.1.1 is already released.

bundfussr · 2024-11-26T10:16:05Z

NEWS.md

+- `order()` function has been added to `event_source()` and `censor_source()` and
+  defaulted to `NULL` to allow sorting of input data. (#2481)


Suggested change

- `order()` function has been added to `event_source()` and `censor_source()` and

defaulted to `NULL` to allow sorting of input data. (#2481)

- `order` argument has been added to `event_source()` and `censor_source()` and

defaulted to `NULL` to allow specifying variables in addition to the date variable.

This can be used to ensure the uniqueness of the select records if there is more

than one record per date. (#2481)

bundfussr · 2024-11-26T10:21:44Z

NEWS.md

+- `check_type = "warning"` default argument added to `derive_param_tte` with an
+  `arg_match` function within the function so the user can use a valid input of 
+  `error, message, warning, or none`. `signal_duplicate_records()` has also been
+  added to the function on lines 394 and 411 to check for uniqueness of records. (#2481)


Could you rephrase this item such that it describes how the change affects the users? No need to add details for developers.

bundfussr · 2024-11-26T10:24:20Z

R/derive_param_tte.R

+                             subject_keys = get_admiral_option("subject_keys"),
+                             check_type = "warning") {
+  # Match check_type to valid admiral options
+  check_type <- rlang::arg_match(check_type, c("warning", "message", "error", "none"))


Please use the admiraldev assertions (assert_character_scalar()) and move it to the "checking and quoting" section below.

ProfessorP-beep added 6 commits November 17, 2024 19:52

Added order arguments to censor_source and event_source. Also added s…

8e23388

…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.

Added order argument to tte_source as part of development and error f…

cd52801

…ixes.

Fixed previous erros but still need to address failed tests for Test …

2727736

…9, 15, and 16 in test-derive_param_tte

added check_type arg_match to derive_param_tte so user has to input a…

9e86217

… valid argument

Changed position of signal_duplicate_records function in derive_param…

d97377c

…_tte to fix missing data error

lintr changes by removing whitespace.

fa49a51

ProfessorP-beep linked an issue Nov 18, 2024 that may be closed by this pull request

Bug: The result of derive_param_tte() depends on the sort order of the input #2481

Open

bms63 reviewed Nov 18, 2024

View reviewed changes

styler fix.

01e8f5a

Pushing again and confirmed check_type argument is in derive_var_obs_number in derive_joined.R scripts

bundfussr reviewed Nov 18, 2024

View reviewed changes

bundfussr reviewed Nov 19, 2024

View reviewed changes

tests/testthat/test-derive_param_tte.R Outdated Show resolved Hide resolved

tests/testthat/test-derive_param_tte.R Outdated Show resolved Hide resolved

ProfessorP-beep and others added 4 commits November 19, 2024 10:54

updated NEWS.md with changes to derive_param_tte,. Removed Test 17 fr…

53457c2

…om test-derive_param_tte as it was redundant, and ran pharmaverse4devs format test script addin to format testest-derive_param_tte.

Merge branch 'main' into 2481-bug-the-result-of-derive_param_tte-depe…

020c9d7

…nds-on-the-sort-order-of-the-input

changed the signal_duplicate_records within derive_parame_tte to hand…

dccdbe1

…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

Merge branch '2481-bug-the-result-of-derive_param_tte-depends-on-the-…

8006891

…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

bundfussr requested changes Nov 19, 2024

View reviewed changes

tests/testthat/test-derive_param_tte.R Outdated Show resolved Hide resolved

R/derive_param_tte.R Outdated Show resolved Hide resolved

ProfessorP-beep added 2 commits November 21, 2024 02:28

added a tryCatch() to filter_date_sources to catch duplicates to addr…

4c95243

…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.

bms63 reviewed Nov 25, 2024

View reviewed changes

bundfussr requested changes Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2481 bug the result of derive param tte depends on the sort order of the input #2569

2481 bug the result of derive param tte depends on the sort order of the input #2569

ProfessorP-beep commented Nov 18, 2024 •

edited

Loading

ProfessorP-beep commented Nov 18, 2024 •

edited

Loading

bms63 Nov 18, 2024

ProfessorP-beep Nov 18, 2024

ProfessorP-beep Nov 18, 2024 •

edited

Loading

ProfessorP-beep Nov 18, 2024

bms63 commented Nov 18, 2024

bundfussr Nov 18, 2024 •

edited

Loading

ProfessorP-beep Nov 18, 2024 •

edited

Loading

bundfussr Nov 18, 2024

ProfessorP-beep Nov 19, 2024

ProfessorP-beep Nov 19, 2024

ProfessorP-beep commented Nov 25, 2024

bms63 left a comment

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

bundfussr Nov 26, 2024

		PARAM = past("Time to First", AEDECOD, "Adverse Event"),
		PARAM = paste("Time to First", AEDECOD, "Adverse Event"),

		- `order()` function has been added to `event_source()` and `censor_source()` and
		defaulted to `NULL` to allow sorting of input data. (#2481)

-- `order()` function has been added to `event_source()` and `censor_source()` and
-  defaulted to `NULL` to allow sorting of input data. (#2481)
+- `order` argument has been added to `event_source()` and `censor_source()` and
+  defaulted to `NULL` to allow specifying variables in addition to the date variable.
+  This can be used to ensure the uniqueness of the select records if there is more
+  than one record per date.  (#2481)

2481 bug the result of derive param tte depends on the sort order of the input #2569

Are you sure you want to change the base?

2481 bug the result of derive param tte depends on the sort order of the input #2569

Conversation

ProfessorP-beep commented Nov 18, 2024 • edited Loading

ProfessorP-beep commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProfessorP-beep Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bms63 commented Nov 18, 2024

bundfussr Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

ProfessorP-beep Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProfessorP-beep commented Nov 25, 2024

bms63 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProfessorP-beep commented Nov 18, 2024 •

edited

Loading

ProfessorP-beep commented Nov 18, 2024 •

edited

Loading

ProfessorP-beep Nov 18, 2024 •

edited

Loading

bundfussr Nov 18, 2024 •

edited

Loading

ProfessorP-beep Nov 18, 2024 •

edited

Loading