Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add rspt01 template #497

Merged
merged 28 commits into from
May 15, 2023
Merged

Add rspt01 template #497

merged 28 commits into from
May 15, 2023

Conversation

Teninq
Copy link
Contributor

@Teninq Teninq commented May 6, 2023 •
edited by clarkliming
Loading

close #471

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2023 •
edited
Loading

🧪 $Test coverage: 97.06%$

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  --------------------------------------------------------------
R/ael01_nollt.R                 26       2  92.31%   82-91
R/aet01_aesi.R                 189       7  96.30%   51, 53-57, 285
R/aet01.R                      342       5  98.54%   41, 253, 328, 334, 562
R/aet02.R                      235       1  99.57%   493
R/aet03.R                       83       0  100.00%
R/aet04.R                      106       2  98.11%   169, 186
R/aet10.R                       50       1  98.00%   102
R/assertions.R                  71       5  92.96%   178-182
R/checks.R                      20       0  100.00%
R/chevron_tlg-S4class.R         21       0  100.00%
R/chevron_tlg-S4methods.R      122      12  90.16%   396-468
R/cmt01a.R                     186       0  100.00%
R/cmt02_pt.R                    49       0  100.00%
R/coxt02.R                      53       1  98.11%   151
R/dmt01.R                       30       0  100.00%
R/dst01.R                      290       0  100.00%
R/dtht01.R                      98       0  100.00%
R/egt01.R                       45       0  100.00%
R/egt02.R                       56       0  100.00%
R/egt03.R                      132       3  97.73%   118, 164, 322
R/egt05_qtcat.R                 57       0  100.00%
R/ext01.R                       71       4  94.37%   230-231, 235-236
R/kmg01.R                       37       7  81.08%   58-66, 106
R/lbt01.R                       94       0  100.00%
R/lbt04.R                       49       0  100.00%
R/lbt05.R                       68       5  92.65%   148-153
R/lbt07.R                       90       1  98.89%   171
R/lbt14.R                      188      24  87.23%   54, 56, 108-109, 111-117, 144, 257, 259, 311-312, 314-320, 347
R/mht01.R                       69       2  97.10%   33-34
R/mng01.R                       95      12  87.37%   114, 118-121, 130-138, 180
R/pdt01.R                       57       0  100.00%
R/pdt02.R                       65       0  100.00%
R/rmpt01.R                      48       2  95.83%   91, 128
R/rspt01.R                      86       3  96.51%   147-150
R/utils.R                       83       2  97.59%   293, 304
R/vst01.R                       47       0  100.00%
R/vst02.R                       92       2  97.83%   109, 240
TOTAL                         3500     103  97.06%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  -------
R/rspt01.R      +86      +3  +96.51%
R/utils.R        +3      +1  -1.16%
TOTAL           +89      +4  -0.04%

Results for commit: f7fcbab3943aed5cea631edec2eb5d7a8cd1640f

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2023 •
edited
Loading

Unit Tests Summary

    1 files    36 suites   1m 54s ⏱️
193 tests 140 ✔️   53 💤 0 ❌
375 runs  249 ✔️ 126 💤 0 ❌

Results for commit 30d90d9.

♻️ This comment has been updated with latest results.

@clarkliming clarkliming self-requested a review May 6, 2023 08:51
clarkliming
clarkliming reviewed May 6, 2023
View reviewed changes
R/rspt01.R Outdated
#' @param dataset (`string`) the name of a table in the `adam_db` object.
#' @param ref_group (`string`) The name of the reference group, the value should
#' be identical to the values in `arm_var`, if not speficied, it will by default
#' use the first level of.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' use the first level of.
#' use the first level or value of `arm_var`.

clarkliming
clarkliming reviewed May 6, 2023
View reviewed changes
R/rspt01.R Outdated
checkmate::assert_flag(odds_ratio)
checkmate::assert_flag(strat_analysis)

arm_level <- unique(anl[[arm_var]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference

chevron/R/utils.R

Line 298 in a91d363

lvls <- function(x) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually it will be a factor

clarkliming
clarkliming reviewed May 6, 2023
View reviewed changes
R/rspt01.R Outdated
}
} else {
lyt02 <- basic_table(show_colcounts = TRUE) %>%
split_cols_by(var = "ARM", ref_group = "A: Drug X") %>%
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref_group =

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and ideally we don't need to repeat that whole part of layout.

clarkliming
clarkliming reviewed May 7, 2023
View reviewed changes
R/rspt01.R Outdated
#' @param strat_analysis (`flag`) should the stratified analysis be performed,
#'
#' @export
proportion_lyt <- function(lyt, arm_var, methods, strata, conf_level, odds_ratio = TRUE, strat_analysis = FALSE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strat_analysis is dependent on strata. If strat_analysis = TRUE but strata is null there could be issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should remove the strat_analysis argument and use strata only? Or we add an assertion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this function I do think we should remove the strata_analysis argument.

clarkliming
clarkliming reviewed May 8, 2023
View reviewed changes
clarkliming
clarkliming reviewed May 8, 2023
View reviewed changes
clarkliming
clarkliming reviewed May 8, 2023
View reviewed changes
clarkliming
clarkliming reviewed May 8, 2023
View reviewed changes
clarkliming
clarkliming reviewed May 8, 2023
View reviewed changes
R/rspt01.R Outdated
estimate_odds_ratio(
vars = "is_rsp",
variables = if (!strat_analysis) list(strata = NULL, arm = NULL) else list(strata = strata, arm = arm_var),
table_names = if (!strat_analysis) "est_or" else "est_or_strat"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will it be easier if we use paste("est_or", suf, sep = "_") where suf = "strat" ? just an idea. very very minor and please ignore if not suitable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, but I prefer to align with other places to be easily understood.

clarkliming
clarkliming reviewed May 9, 2023
View reviewed changes
clarkliming
clarkliming reviewed May 9, 2023
View reviewed changes
@Teninq Teninq requested a review from clarkliming May 10, 2023 08:18
clarkliming
clarkliming approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@clarkliming clarkliming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. thanks

@Teninq Teninq requested a review from clarkliming May 12, 2023 11:11
clarkliming
clarkliming reviewed May 15, 2023
View reviewed changes
R/rspt01.R
checkmate::assert_string(ref_group, null.ok = TRUE)
checkmate::assert_flag(odds_ratio)
checkmate::assert_subset(perform_analysis, c("unstrat", "strat"))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check strata here (early is better than late)

Suggested change
checkmate::assert_character(strata, null.ok = !"stata" %in% perform_analysis)

clarkliming
clarkliming reviewed May 15, 2023
View reviewed changes
R/rspt01.R Outdated
proportion_lyt(
arm_var = arm_var,
odds_ratio = odds_ratio,
strata = strata1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
strata = strata1,
if (perform == "strat") strata

this should work

@Teninq Teninq requested a review from clarkliming May 15, 2023 07:12
clarkliming
clarkliming approved these changes May 15, 2023
View reviewed changes
Copy link
Contributor

@clarkliming clarkliming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thank you

@clarkliming clarkliming merged commit d2f17b5 into main May 15, 2023
@clarkliming clarkliming deleted the 471_rspt01_template branch May 15, 2023 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clarkliming clarkliming clarkliming approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RSPT01 [Spec Added]
2 participants
@Teninq @clarkliming