-
Notifications
You must be signed in to change notification settings - Fork 71
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 #1984 Allowing missing trt end date in derive_var_ontrtfl()
#2029
Changes from 8 commits
c707bbb
1e55406
e493c38
f1147d7
8b3efab
31ea38b
01e315f
55bec4e
fbd437c
50f3d2c
550c6d2
120236b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
ddsjoberg marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# derive_var_ontrtfl Test 15: if trt end date is missing, the obs may still be flagged | ||
|
||
Code | ||
derive_var_ontrtfl(adcm, start_date = ASTDT, end_date = AENDT, ref_start_date = TRTSDT, | ||
ref_end_date = TRTEDT, span_period = "Y") | ||
Output | ||
USUBJID ASTDT TRTSDT TRTEDT AENDT ONTRTFL | ||
1 P01 2018-03-15 2019-01-01 NA 2022-12-01 Y | ||
2 P02 2020-04-30 2019-01-01 NA 2022-03-15 Y | ||
3 P03 2020-04-30 2019-01-01 NA <NA> Y | ||
|
||
--- | ||
|
||
Code | ||
derive_var_ontrtfl(adcm, start_date = ASTDT, end_date = AENDT, ref_start_date = TRTSDT, | ||
ref_end_date = TRTEDT) | ||
Output | ||
USUBJID ASTDT TRTSDT TRTEDT AENDT ONTRTFL | ||
1 P01 2018-03-15 2019-01-01 NA 2022-12-01 <NA> | ||
2 P02 2020-04-30 2019-01-01 NA 2022-03-15 Y | ||
3 P03 2020-04-30 2019-01-01 NA <NA> Y | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the two snapshots generated are seen as the truth. If there is a change that impacts this function, when the test suite is run again and new snapshot is different from the old snapshot then it will fail?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's exactly right! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pharmaverse/admiral hey all here is a good example of using snapshots in our tests. |
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.
Many thanks for doing this!!
In the future, can you not fork and just branch within admiral repo. I like to pull down the branch and render the documentation locally for a final inspection before approving.
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.
sure, that's no problem.
FYI, PRs have a PR branch in the repo that is equal to the external branch that is being merged. You can interact with that branch just like any other (pulling down, building docs, pushing to, etc.)

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.
Oh well…TIL - please disregard my request
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.
Well I was able to fetch the forked branch thing and render the documenation - realized there is only the changelog
Pretty cool to have this option.