Closes #2302 New parameter, group_var added

[ashachakma]

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.

• [x] Place Closes # into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
• [x] Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
• [x] Updated relevant unit tests or have written new unit tests, which should consider realistic data scenarios and edge cases, e.g. empty datasets, errors, boundary cases etc. - See Unit Test Guide
• [x] If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
• [x] Review the Cheat Sheet. Make any required updates to it by editing the file inst/cheatsheet/admiral_cheatsheet.pptx and re-upload a PDF version of it to the same folder.
• [x] Update to all relevant roxygen headers and examples, including keywords and families. Refer to the categorization of functions to tag appropriate keyword/family.
• [x] Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• [x] Address any updates needed for vignettes and/or templates
• [x] Update NEWS.md under the header # admiral (development version) if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers)
• [x] Build admiral site pkgdown::build_site() and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.
• [x] Address or fix all lintr warnings and errors - lintr::lint_package()
• [x] Run R CMD check locally and address all errors and warnings - devtools::check()
• [x] Link the issue in the Development Section on the right hand side.
• [x] Address all merge conflicts and resolve appropriately
• [x] Pat yourself on the back for a job well done! Much love to your accomplishment!

[github-actions[bot]]

Package	Line Rate	Health
admiral	96%	✔
Summary	96% (4859 / 5058)	✔

[bms63]

@kaz462 do you have some time this week to review this update?

[bms63]

Hi @kaz462 - does this PR's core code meet your expectations for your issue? I think we need some more tests

[nbrucken17]

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.

[kaz462]

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?

[bms63]

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?

[bms63]

Hi @ashachakma - Is Option 1 already implemented?

[ashachakma]

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.

[github-actions[bot]]

This Pull Request is stale because it has not been worked on in 15 days.

[bms63]

@ashachakma we need you! Can you complete this soon! EOW?

[ashachakma]

@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!

[ashachakma]

Just FYI - I am still working on this PR.

[ashachakma]

Hi, I am getting this issue while running devtools::check(). These are not from the tests that I have updated, can anyone help me with this?

[manciniedoardo]

Hi, I am getting this issue while running devtools::check(). These are not from the tests that I have updated, can anyone help me with this?

@ashachakma what is your installed version of admiraldev? `packageVersion("admiraldev")

[bundfussr]

Hi, I am getting this issue while running devtools::check(). These are not from the tests that I have updated, can anyone help me with this?

This should be resolved by merging main into your feature branch.

[ashachakma]

packageVersion("admiraldev") [1] ‘1.0.0.9027’

[ashachakma]

Hi @ashachakma - Is Option 1 already implemented?

@bms63 @kaz462 It is ready for review now :)

[bms63]

Hi @ashachakma are you able to finish by the EOD Friday? @kaz462 is on west coast of US so should have time to review and we can work together to do any final minor updates.

[ashachakma]

Hi @ashachakma are you able to finish by the EOD Friday? @kaz462 is on west coast of US so should have time to review and we can work together to do any final minor updates.

Hi Ben, discussions are ongoing and I will need some time to implement the new changes. I don't think today EOD is possible given my study dry run is also running in parallel.

[bms63]

Totally understand @ashachakma !! You have gotten us this far!!

@kaz462 are you able to complete this for @ashachakma today? Please let me know ASAP

[bms63]

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.

[bms63]

wheew...my head hurts. I saw a diagram floating around. Perhaps in 1.2.0 we could publish the diagram in inst folder for folks to reference to? Would that be useful for the users? IT would be for me, but I'm a bit dull!! If useful, do you mind making an issue and placing the diagram in the issue. We can clean it up or remake but just as a placeholder for now.

So close to being done!! yay!!

[bms63]

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?

[bundfussr]

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 initial_intensity shouldn't be specified.

We should update the example.

[bms63]

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 initial_intensity shouldn't be specified.

We should update the example.

Good point!! - should the warning message tell the user to remove initial_intensity

[bundfussr]

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 initial_intensity shouldn't be specified. We should update the example.

Good point!! - should the warning message tell the user to remove initial_intensity

We don't know if initial_intensity or group_var should be removed. Thus we could add "Please only specify one of them." to the message.

[bms63]

@kaz462 do you some time? Just a few minor tweaks.

[kaz462]

@kaz462 do you some time? Just a few minor tweaks.

Yes, will update later today

[kaz462]

@ashachakma @bms63 @bundfussr Thanks for your review! I've addressed the new comments, please let me know if I missed anything.

@bms63 Issue created to improve the documentation: https://github.com/pharmaverse/admiral/issues/2455

[bms63]

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

[bms63]

Yes!!!