Closes #234 Add new ADRS vignette

[bundfussr]

Please add "Closes #" to the title of the pull request. Then the
issue is closed automatically once it is merged to main.

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

• Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
• Updated relevant unit tests or have written new unit tests - See Unit Test Guide
• If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
• Update to all relevant roxygen headers and examples
• Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• Address any updates needed for vignettes and/or templates
• Update NEWS.md
• 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.
• Address or fix all lintr warnings and errors - lintr::lint_package()
• Run R CMD check locally and address all errors and warnings - devtools::check()
• Link the issue so that it closes after successful merging.
• Address all merge conflicts and resolve appropriately
• Pat yourself on the back for a job well done! Much love to your accomplishment!

The following line requests to update the man pages by the "Man Pages" workflow.

/roxygenize

[github-actions]

Package	Line Rate	Health
admiralonco	97%	✔
Summary	97% (542 / 558)	✔

[rossfarrugia]

@bundfussr leaving first batch of review comments. i need to run pkgdown next as easier to review the vignettes that way, so may have some more to add.

@amitjaingsk i'm focusing my review mostly on the documentation so trust you and the other team members are reviewing the code here and comparing the new alternative ADRS code against the basic version we have to ensure consistent results over some different data.

[rossfarrugia]

few more comments for consideration - and still working through reviewing the new vignette

[rossfarrugia]

final review comments from me for this PR

[rossfarrugia]

@bundfussr do you think users might get confused that we have event_sources.R that runs event() to create ADRS objects, and tte_sources.R that runs event_source() to create ADTTE objects. I just got myself in a tangle trying to understand the difference between event_sources and event_source, and then figured it out. I don't have any alternative naming solution to suggest either though 😕

[bundfussr]

I forgot to mention that the new admiral functionality is not merged to devel yet. If you want to render the new vignette, you need to call remotes::install_github("pharmaverse/admiral", ref = "1960_enhance_derive_extreme_event@devel") for installing admiral first.

[bundfussr]

@bundfussr do you think users might get confused that we have event_sources.R that runs event() to create ADRS objects, and tte_sources.R that runs event_source() to create ADTTE objects. I just got myself in a tangle trying to understand the difference between event_sources and event_source, and then figured it out. I don't have any alternative naming solution to suggest either though 😕

Good point! I have added it to the clean-up discussion.

[bundfussr]

@amitjaingsk , I have updated the ADRS vignettes and templates such that they are using the new test data from pharmasdtm now.

[amitjaingsk]

Hi Stefan,
I did some review but as not able to run and see the results and vignette. Not able to review thoroughly. But provided few comments please have a look.

[amitjaingsk]

here you mean we only include overall response record and not target or non-target responses at each visit? if right can we update:
It is assumed that dataset_name = "ovr" refers to the dataset of the only overall response assessments at each visit which should be considered for the parameter....
2. I assume we prefer to only consider post-baseline records in 'ovr' dataset.. if yes can you please add as its important. Now in SDTM RS domain, some companies also include prior anti-cancer therapy or screening responses.

[bundfussr]

I have updated the text regarding both points.

[amitjaingsk]

AVALC (and aval) should not be both missing.. I guess we can use 'NE' right. So, the intend is to create this in the end right? I understand it but can we update the description "Define missing response (MISSING) for all patients in adsl in your population who has no post baseline response (should be used", "as last event)"

[bundfussr]

I have updated the text as suggested. You could use AVALC = "NE" instead of AVALC = "MISSING". I used AVALC = "MISSING" because that's used in the basic version and was used in the old vignette.

[bundfussr]

@amitjaingsk , for a rendered version of the website please see https://bundfussr.github.io/admiral/234_deprecate/.

[amitjaingsk]

Thanks for updating Stefan. I was able to run adrs_basic is its looks good, only one suggestion I made. for adrs, I was not able to run so not able to make sure the results are matching is adrs_basic , did you checked? Otherwise it looks good.

[bundfussr]

make sure the results are matching is adrs_basic , did you checked?

I compared the results of the adrs and adrs_basic template. They match (except that adrs additionally creates ANL02FL but this is an expected difference).

[rossfarrugia]

@bundfussr few more comments from re-review

[rossfarrugia]

@bundfussr do you think users might get confused that we have event_sources.R that runs event() to create ADRS objects, and tte_sources.R that runs event_source() to create ADTTE objects. I just got myself in a tangle trying to understand the difference between event_sources and event_source, and then figured it out. I don't have any alternative naming solution to suggest either though 😕

Good point! I have added it to the clean-up discussion.

Suggestion: i think the potentially easiest solution here is in admiralonco call it response_sources.R instead of event_sources.R

[rossfarrugia]

@bundfussr few more minor comments spotted and then happy to approve