-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closes #2154 fix_dthcaus: fix derive_var_dthcaus() #2162
Conversation
DESCRIPTION
Outdated
@@ -1,7 +1,7 @@ | |||
Package: admiral | |||
Type: Package | |||
Title: ADaM in R Asset Library | |||
Version: 0.12.2 | |||
Version: 0.12.2.9000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version: 0.12.2.9000 | |
Version: 0.12.3 |
since this should be a one-time fix, no need to add development flow to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do a another bug fix release. should just ship out with 1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im unclear if CRAN would even accept a third patch in 3 weeks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, we can remove my suggestion, but merge to main
instead of patch
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes merge to main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rossfarrugia , would like to have this fixed soon because the issue was detected by a Roche study using admiral.
Maybe we could do a patch release but do not submit it to CRAN? However, it may be confusing for health authorities if a study does a submission with admiral 0.12.3 and this version is not available on CRAN.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my take on this is its not a minor bug in terms of impact - its a common derivation that pretty much every study will use, it relates to death information which is key safety info, and currently it doesn't work as documented in admiral. so for me thats something i'd not want to wait for as a user. so i'd be in favour of releasing as 0.12.3 and if CRAN don't accept then nothing we can do about that. at least users would have an option to install and use this from github until 1.0.0 comes later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
@bundfussr is this branched off release tag 12.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I branched off patch
, which should be the same as tag 12.2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to 0.12.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small detail, but for the purpose of merging it in relatively soon(?) we can skip the 9000 versioning
NEWS.md
Outdated
- `derive_var_dthcaus()` was fixed. If a subject has observations in more than | ||
one of the sources, the one from the last source was selected regardless of the | ||
date. Now the function works as described in its documentation. (#2154) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `derive_var_dthcaus()` was fixed. If a subject has observations in more than | |
one of the sources, the one from the last source was selected regardless of the | |
date. Now the function works as described in its documentation. (#2154) | |
- Fixed a bug in `derive_var_dthcaus()` where if a subject has observations in more than | |
one of the sources, the one from the last source was selected regardless of the | |
date. Now the function works as described in its documentation. (#2154) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
ds <- tibble::tribble( | ||
~STUDYID, ~USUBJID, ~DSSEQ, ~DSDECOD, ~DSTERM, ~DSSTDTC, | ||
"TEST01", "PAT01", 4, "DEATH", "DEATH DUE TO progression of disease", "2021-04-05", | ||
"TEST01", "PAT02", 1, "INFORMED CONSENT OBTAINED", "INFORMED CONSENT OBTAINED", "2021-04-02", | ||
"TEST01", "PAT02", 2, "RANDOMIZATION", "RANDOMIZATION", "2021-04-11", | ||
"TEST01", "PAT02", 3, "COMPLETED", "PROTOCOL COMPLETED", "2021-12-01", | ||
"TEST01", "PAT03", 1, "DEATH", "DEATH DUE TO progression of disease", "2021-04-07", | ||
"TEST01", "PAT03", 2, "RANDOMIZATION", "RANDOMIZATION", "2021-04-11", | ||
"TEST01", "PAT03", 3, "COMPLETED", "PROTOCOL COMPLETED", "2021-12-01" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to worry about any partial dates here since these are ---DTC
variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess a death would always have an exact date. just musing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to worry. derive_var_dthcaus()
expects dates (not DTC
variables). The users have to handle partial dates in a preprocessing step or on the definition of the source objects (like in the examples).
* Closes #2147 addressing cran failure (#2149) * feat: #2147 one solution but proceed with caution * description uplevel and news blurb * pass links? --------- Co-authored-by: Zelos Zhu <zdz2101@github.com> * Update README.md (#2155) * Update README.md * URL update --------- Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> * Closes #2154 fix_dthcaus: fix derive_var_dthcaus() (#2162) * #2154 fix_dthcaus: fix derive_var_dthcaus() * #2154 fix_dthcaus: run templates check * #2154 fix_dthcaus: update version number and NEWS * docs: fixing curlys after feedback from CRAN * chore: styling death test --------- Co-authored-by: Zelos Zhu <zelos.zhu@atorusresearch.com> Co-authored-by: Zelos Zhu <zdz2101@github.com> Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> Co-authored-by: Stefan Bundfuss <80953585+bundfussr@users.noreply.github.com>
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.styler::style_file()
to style R and Rmd filesdevtools::document()
so all.Rd
files in theman
folder and theNAMESPACE
file in the project root are updated appropriatelyNEWS.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)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 check
locally and address all errors and warnings -devtools::check()