Closes #2302 New parameter, group_var added #2362
Conversation
|
|
@kaz462 do you have some time this week to review this update? |
R/admiral-package.R
Outdated
| #' @importFrom stats setNames | ||
| #' @importFrom stats lag |
There was a problem hiding this comment.
| #' @importFrom stats setNames | |
| #' @importFrom stats lag | |
| #' @importFrom stats setNames lag |
There was a problem hiding this comment.
@ashachakma should we use dplyr::lag instead of stats::lag?
| ## Test 3: considering trt end time ---- | ||
| test_that("derive_var_trtemfl Test 3: considering trt end time", { | ||
| ## Test 3: with end_window and worsening within grouping variable---- | ||
| test_that("derive_var_trtemfl Test 3: with end_window and worsening within grouping variable", { |
There was a problem hiding this comment.
I feel like more tests should be written - that is a lot of new code under the hood that just has one test backing it up!!
What happens if group_var is specified and some of the other arguments are missing or poorly defined?
There was a problem hiding this comment.
Sure Ben, I will add some more tests.
There was a problem hiding this comment.
can we add the following data for testing -
currently, there are some differences between expected2 and comp, do you agree with TEAE_expected values from expected2?
expected2 <- tribble(
~USUBJID, ~ASTDTM, ~AENDTM, ~AEITOXGR, ~AETOXGR, ~AEGRPID, ~TEAE_expected,
# starting before treatment and ending during treatment
"1", "2021-12-30", "2022-01-14", "3", "3", "1", NA,
"1", "2022-01-05", "2022-06-01", "3", "1", "1", NA,
"1", "2022-01-10", "2022-01-11", "3", "2", "1", "Y",
"1", "2022-01-13", "2022-03-01", "3", "1", "1", "Y",
"1", "2021-12-30", "2022-01-14", "2", "2", "2", NA,
"1", "2022-01-05", "2022-06-01", "2", "1", "2", NA,
"1", "2022-01-10", "2022-03-01", "2", "1", "2", NA,
) %>%
mutate(
ASTDTM = lubridate::ymd(ASTDTM),
AENDTM = lubridate::ymd(AENDTM),
TRTSDTM = if_else(USUBJID == "1", lubridate::ymd("2022-01-01"), ymd("")),
TRTEDTM = if_else(USUBJID == "1", lubridate::ymd("2022-04-30"), ymd(""))
)
comp <- derive_var_trtemfl(
expected2,
new_var = TEAE_derived,
trt_end_date = TRTEDTM,
end_window = 10,
initial_intensity = AEITOXGR,
intensity = AETOXGR,
group_var = AEGRPID
)comp:
# A tibble: 7 × 10
USUBJID ASTDTM AENDTM AEITOXGR AETOXGR AEGRPID TRTSDTM TRTEDTM TEAE_expected TEAE_derived
<chr> <date> <date> <chr> <chr> <chr> <date> <date> <chr> <chr>
1 1 2021-12-30 2022-01-14 3 3 1 2022-01-01 2022-04-30 NA NA
2 1 2022-01-05 2022-06-01 3 1 1 2022-01-01 2022-04-30 NA Y
3 1 2022-01-10 2022-01-11 3 2 1 2022-01-01 2022-04-30 Y Y
4 1 2022-01-13 2022-03-01 3 1 1 2022-01-01 2022-04-30 Y Y
5 1 2021-12-30 2022-01-14 2 2 2 2022-01-01 2022-04-30 NA NA
6 1 2022-01-05 2022-06-01 2 1 2 2022-01-01 2022-04-30 NA Y
7 1 2022-01-10 2022-03-01 2 1 2 2022-01-01 2022-04-30 NA Y 
|
Hi @kaz462 - does this PR's core code meet your expectations for your issue? I think we need some more tests |
There was a problem hiding this comment.
@ashachakma Thanks a lot for implementing the grouping argument!
I left some comments but haven't finished the review, will continue after clarifying the current findings
R/admiral-package.R
Outdated
| #' @importFrom stats setNames | ||
| #' @importFrom stats lag |
There was a problem hiding this comment.
@ashachakma should we use dplyr::lag instead of stats::lag?
R/derive_var_trtemfl.R
Outdated
| #' If the argument is specified, events which start before treatment start and | ||
| #' end after treatment start (or are ongoing) and worsened (i.e., the | ||
| #' intensity is greater than the initial intensity), are flagged. |
There was a problem hiding this comment.
| #' If the argument is specified, events which start before treatment start and | |
| #' end after treatment start (or are ongoing) and worsened (i.e., the | |
| #' intensity is greater than the initial intensity), are flagged. | |
| #' If the argument is specified, events which start before treatment start and | |
| #' worsened during treatment are flagged. |
- should we update "end after treatment start (or are ongoing) and worsened ..." to "worsened during treatment"? this way it includes the period between
TRTSDTtoTRTEDT+end_window - should we remove or rephrase the following:
" (i.e., the intensity is greater than the initial intensity)"
as shown in the third record below, the intensity is not greater than the initial intensity and it's still flagged
There was a problem hiding this comment.
- Yeah, I agree with your suggestion. Will update it to "worsened during treatment".
- Would be good to rephrase it- " If the argument is specified, events which started during the treatment period irrespective of worsening condition or events which start before treatment start and end after treatment start (or are ongoing) and worsened during treatment (i.e., the intensity is greater than the initial intensity), are flagged." Let me know if you agree with this.
R/derive_var_trtemfl.R
Outdated
| group_by(USUBJID, !!group_var) %>% | ||
| mutate(worsen_date = case_when( | ||
| start_date >= trt_start_date & | ||
| (!!intensity > !is.na(!!initial_intensity)) ~ as_datetime(!!start_date), |
There was a problem hiding this comment.
| (!!intensity > !is.na(!!initial_intensity)) ~ as_datetime(!!start_date), | |
| (!!intensity > lag(!!intensity)) ~ as_datetime(!!start_date), |
from #2302 (comment) step 1: compare with the previous record after TRTSDT ordered by ASTDT
There was a problem hiding this comment.
lag() gives NA for the first record, so it would miss those where 1st record is a worsened one (tried a random example).
I was considering that AEITOXGR will remain consistent for a group ID, it might be more convenient to compare with AEITOXGR to identify the worsened records. What do you think?
There was a problem hiding this comment.
I think we need to clarify the definition of "worsened" - do you think the 3rd and 4th records should be flagged as TEAE in comp from here?
- The change of
AETOXGRfor this episode of AE: 3->1->2->1 - The 3rd record has
AETOXGR=2,AEITOXGR=3, and its previous record hasAETOXGR=1. If we compare it with the previous record, the 3rd record is worsened, but it's improving if we compare it withAEITOXGR. - If we assume
AEITOXGRis derived from the first record within eachgroup_var, i.e. the first record hasAEITOXGR = AETOXGR, then it's okay to haveNAfor the first record as no comparison is necessary.
The TEAE_expected values below are just my opinion, please let me know your thoughts. Thanks for delving into this!
# A tibble: 7 × 10
USUBJID ASTDTM AENDTM AEITOXGR AETOXGR AEGRPID TRTSDTM TRTEDTM TEAE_expected TEAE_derived
<chr> <date> <date> <chr> <chr> <chr> <date> <date> <chr> <chr>
1 1 2021-12-30 2022-01-14 3 3 1 2022-01-01 2022-04-30 NA NA
2 1 2022-01-05 2022-06-01 3 1 1 2022-01-01 2022-04-30 NA Y
3 1 2022-01-10 2022-01-11 3 2 1 2022-01-01 2022-04-30 Y Y
4 1 2022-01-13 2022-03-01 3 1 1 2022-01-01 2022-04-30 Y Y There was a problem hiding this comment.
Thanks @kaz462 for clarifying my doubts and apologies for the delayed response -
- I agree with the 3rd record being flagged as TEAE since its AE severity worsened from 1>2. Comparing it with the previous record makes more sense.
- For 4th record, I think it shouldn't be flagged since the severity improved within the trt window (I remember we agreed before, that any record after the worsened record should be flagged as TEAE, but I was referring to this paper again and it mentions how to handle this scenario. Pls refer to scenario 4 and let me know if you agree to this- TEAE: Did I flag it right?
There was a problem hiding this comment.
Pls refer to scenario 4 and let me know if you agree to this- TEAE: Did I flag it right?
I agree with not flagging the 2nd record (improved). For the 4th record, this type (worsened then improved) is flagged as TEAE in the study I'm working on, similar to the ASEQ =6 record discussed here. We may need to re-visit this decision:
- Once a record within a grouped AE is flagged as a TEAE, then any subsequent record during treatment should also be a TEAE regardless of the severity
Options to consider are:
- stay with the current decision
- the 4th record shouldn't be flagged as it has improved compared to the previous one
- add more flexibility to accommodate the above two scenarios
- details to be discussed...
# A tibble: 7 × 10
USUBJID ASTDTM AENDTM AEITOXGR AETOXGR AEGRPID TRTSDTM TRTEDTM
<chr> <date> <date> <chr> <chr> <chr> <date> <date>
1 1 2021-12-30 2022-01-14 3 3 1 2022-01-01 2022-04-30
2 1 2022-01-05 2022-06-01 3 1 1 2022-01-01 2022-04-30
3 1 2022-01-10 2022-01-11 3 2 1 2022-01-01 2022-04-30
4 1 2022-01-13 2022-03-01 3 1 1 2022-01-01 2022-04-30 @pharmaverse/admiral I added this topic to the next meeting agenda
There was a problem hiding this comment.
At Roche AEITOXGR is the initial grade, i.e., the grade observed when the AE started regardless if it was before or after treatment start. However, this is Roche specific, AEITOXGR is not a CDSIC SDTM variable.
Why do we need AEITOXGR if group_var is specified? We could select the last record before treatment start and all records thereafter. If any worsening (compared to the previous record) occurs, the record is flagged (and all records after that). Did I miss something?
There was a problem hiding this comment.
If we combine the following AEs into a single record -
USUBJID AEGRPID ASTDT AENDT AETOXGR TRTSDT TRTEDT
<chr> <chr> <date> <date> <chr> <date> <date>
1 1 2 2021-12-30 2022-01-04 2 2022-01-10 2022-04-30
2 1 2 2022-01-05 2022-01-09 3 2022-01-10 2022-04-30
3 1 2 2022-01-10 2022-03-01 3 2022-01-10 2022-04-30- Use the first record before treatment
AEITOXGR=2as the initial intensity , the combined AE below should be flagged as TEAE, same for the 3rd AE above.
USUBJID AEGRPID ASTDT AENDT AEITOXGR AETOXGR TRTSDT TRTEDT TRTEMFL
<chr> <chr> <date> <date> <chr> <chr> <date> <date> <chr>
1 1 2 2021-12-30 2022-03-01 2 3 2022-01-10 2022-04-30 Y - Use the last record before treatment
AEITOXGR=3as the initial intensity, the combined AE below shouldn't be flagged as TEAE, same for the 3rd AE above.
USUBJID AEGRPID ASTDT AENDT AEITOXGR AETOXGR TRTSDT TRTEDT TRTEMFL
<chr> <chr> <date> <date> <chr> <chr> <date> <date> <chr>
1 1 2 2021-12-30 2022-03-01 3 3 2022-01-10 2022-04-30 NA The current derive_var_trtemfl determines worsened record by comparing the intensity with initial_intensity. Should initial_intensity/AEITOXGR be interpreted as pre-treatment status in the function, and with the same definition regardless of AE data collection method?
When determining pre-treatment status, should we always use the last record before treatment, or allow flexibility?
There was a problem hiding this comment.
Should
initial_intensity/AEITOXGRbe interpreted as pre-treatment status in the function, and with the same definition regardless of AE data collection method?
I would interpret initial_intensity/AEITOXGR as the grade/intensity when the AE started. I would use it only if a single record per AE is collected. If a separate records is collected for each change in grade/intensity, I would ignore initial_intensity/AEITOXGR and just use intensity/AETOXGR. This seems easier and clearer to me.
When determining pre-treatment status, should we always use the last record before treatment, or allow flexibility?
Using the last record seems reasonable to me. I am not aware of a use case where I would deviate from this. Thus I would not allow flexibility for now.
There was a problem hiding this comment.
Determining TEAEs is definitely not as easy as it looks upon first glance. I don't know if it will help, but the PHUSE/CS Working Group on TEAEs published this poster last fall- https://www.lexjansen.com/css-us/2023/POS_PP09.pdf. It contains results of a survey they conducted, with responses from people in a variety of the disciplines involved in clinical trials, and as you can see, there were several scenarios without any sort of general agreement as to whether the AEs in question should be considered as treatment-emergent. Maybe the best thing to do is make sure the situations where there was broad agreement are being handled correctly, and then pick a direction for the rest, but be sure it's documented somewhere how those have been resolved, and allow a way to override the default behavior since there isn't a standard industry approach in those cases.
There was a problem hiding this comment.
As discussed in the meeting, we have decided to implement the group_var feature based on option 1 for now.
We can cross-check the functionality with two white papers when they become available, and add features as needed later. @nbrucken17 please keep us posted, thanks :)
https://advance.phuse.global/display/WEL/Treatment+Emergent+Definitions+Recommendations
https://advance.phuse.global/display/WEL/Adverse+Event+Collection+Recommendations
| ## Test 3: considering trt end time ---- | ||
| test_that("derive_var_trtemfl Test 3: considering trt end time", { | ||
| ## Test 3: with end_window and worsening within grouping variable---- | ||
| test_that("derive_var_trtemfl Test 3: with end_window and worsening within grouping variable", { |
There was a problem hiding this comment.
can we add the following data for testing -
currently, there are some differences between expected2 and comp, do you agree with TEAE_expected values from expected2?
expected2 <- tribble(
~USUBJID, ~ASTDTM, ~AENDTM, ~AEITOXGR, ~AETOXGR, ~AEGRPID, ~TEAE_expected,
# starting before treatment and ending during treatment
"1", "2021-12-30", "2022-01-14", "3", "3", "1", NA,
"1", "2022-01-05", "2022-06-01", "3", "1", "1", NA,
"1", "2022-01-10", "2022-01-11", "3", "2", "1", "Y",
"1", "2022-01-13", "2022-03-01", "3", "1", "1", "Y",
"1", "2021-12-30", "2022-01-14", "2", "2", "2", NA,
"1", "2022-01-05", "2022-06-01", "2", "1", "2", NA,
"1", "2022-01-10", "2022-03-01", "2", "1", "2", NA,
) %>%
mutate(
ASTDTM = lubridate::ymd(ASTDTM),
AENDTM = lubridate::ymd(AENDTM),
TRTSDTM = if_else(USUBJID == "1", lubridate::ymd("2022-01-01"), ymd("")),
TRTEDTM = if_else(USUBJID == "1", lubridate::ymd("2022-04-30"), ymd(""))
)
comp <- derive_var_trtemfl(
expected2,
new_var = TEAE_derived,
trt_end_date = TRTEDTM,
end_window = 10,
initial_intensity = AEITOXGR,
intensity = AETOXGR,
group_var = AEGRPID
)comp:
# A tibble: 7 × 10
USUBJID ASTDTM AENDTM AEITOXGR AETOXGR AEGRPID TRTSDTM TRTEDTM TEAE_expected TEAE_derived
<chr> <date> <date> <chr> <chr> <chr> <date> <date> <chr> <chr>
1 1 2021-12-30 2022-01-14 3 3 1 2022-01-01 2022-04-30 NA NA
2 1 2022-01-05 2022-06-01 3 1 1 2022-01-01 2022-04-30 NA Y
3 1 2022-01-10 2022-01-11 3 2 1 2022-01-01 2022-04-30 Y Y
4 1 2022-01-13 2022-03-01 3 1 1 2022-01-01 2022-04-30 Y Y
5 1 2021-12-30 2022-01-14 2 2 2 2022-01-01 2022-04-30 NA NA
6 1 2022-01-05 2022-06-01 2 1 2 2022-01-01 2022-04-30 NA Y
7 1 2022-01-10 2022-03-01 2 1 2 2022-01-01 2022-04-30 NA Y 
|
For what it's worth, there is some discussion happening right now over how AEs should be collected, and thus how TEAE flagging should work. There are 2 PHUSE WGs developing white papers; the AE Collection Recommendations WG (https://advance.phuse.global/display/WEL/Adverse+Event+Collection+Recommendations) is currently incorporating public review comments, while the TE Definitions Recommendations (https://advance.phuse.global/display/WEL/Treatment+Emergent+Definitions+Recommendations) WG should have their white paper out for public review sometime in the next few months. However, both WGs have found considerable variation across sponsors both in how AEs are collected (single event at max severity, every time severity worsens, every time severity changes, etc.), and how TEs are defined/determined. So there's definitely a need for flexibility in how this function handles various scenarios- or maybe even multiple functions to handle the current variety of approaches. |
@nbrucken17 Thanks for your information and connecting the dots here! A summary of different AE collection methods and TEAE scenarios across sponsors would be very helpful. Maybe we can revisit this topic after the white paper is out? |
This is great @nbrucken17 thank you for chiming in. @kaz462 I would still be interested in implementing if it doesn't impact the original functionality - my understanding is that this is the case. However, maybe a separate function(s) could be explored? |
|
Hi @ashachakma - Is Option 1 already implemented? |
Hi Ben, I've been holding off on implementing the updates for option 1 until receiving this confirmation. I will work on those updates by this weekend. |
|
This Pull Request is stale because it has not been worked on in 15 days. |
|
@ashachakma we need you! Can you complete this soon! EOW?
|
Sorry Ben, this week I've a planned holiday starting from Friday till next Tuesday 🫣 I was working on this issue intermittently but due to study work, not been able to finish it yet. I am at the testing stage but issues are there as well. I will prioritize this task next week for sure! |
|
Ohhh - this is making me pause. Is this necessary as you are just grabbing USUBJID? Is there a scenario where a user will need something more and should we give them so much leeway? If it is needed, can we recommend for them to check out set_admiral_opiton() and get_admiral_option() in the argument documentation.
|
|
wheew...my head hurts. I saw a diagram floating around. Perhaps in 1.2.0 we could publish the diagram in So close to being done!! yay!! |
|
Should we add ability to suppress the warning message if a user wants this? Or should it just be a message to the user instead of a warning?
|
I don't think we need an extra argument for this. To avoid the warning We should update the example. |
Good point!! - should the warning message tell the user to remove |
We don't know if |
|
@kaz462 do you some time? Just a few minor tweaks. |
Yes, will update later today |
|
@ashachakma @bms63 @bundfussr Thanks for your review! I've addressed the new comments, please let me know if I missed anything. |
|
@kaz462 Stefan tweaked the code a bit. Do you mind doing quick review (if you have time) and then I will look over as well and we can merge in. |
|
Yes!!!
|
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 version of it to the same folder.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)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()