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

implement egt03 #347

Merged
merged 10 commits into from
Jan 12, 2023
Merged

implement egt03 #347

merged 10 commits into from
Jan 12, 2023

Conversation

shajoezhu
Copy link
Contributor

close insightsengineering/chevron-tasks#51

@shajoezhu shajoezhu added the sme label Jan 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023 •
edited
Loading

Unit Tests Summary

    1 files    18 suites   1m 0s ⏱️
  89 tests   60 ✔️ 29 💤 0 ❌
192 runs  120 ✔️ 72 💤 0 ❌

Results for commit 22a76a8.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023 •
edited
Loading

🧪 $Test coverage: 89.36%$

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ---------------------------------
R/aet01.R                      330       6  98.18%   38, 240, 253, 316, 322, 554
R/aet02.R                      222       2  99.10%   136, 489
R/aet03.R                       85       0  100.00%
R/aet04.R                      104       6  94.23%   81-86
R/assertions.R                  36       0  100.00%
R/checks.R                      14       0  100.00%
R/chevron_tlg-S4class.R         21       0  100.00%
R/chevron_tlg-S4methods.R       42      16  61.90%   47-55, 90-98, 218-290
R/cmt01a.R                     191       0  100.00%
R/cmt02_pt.R                    52       0  100.00%
R/dmt01.R                       40       0  100.00%
R/dst01.R                      287       0  100.00%
R/dtht01.R                     104       0  100.00%
R/egt01.R                       46       0  100.00%
R/egt02.R                       60       0  100.00%
R/egt03.R                      129      85  34.11%   32-52, 111, 125-232, 291, 305-339
R/ext01.R                       81       8  90.12%   238-241, 245-248
R/gen_args.R                     1       1  0.00%    27
R/lbt01.R                       44       0  100.00%
R/lbt04.R                       52       1  98.08%   127
R/mht01.R                       72       2  97.22%   33-34
R/mng01.R                       93       9  90.32%   113, 117-120, 133-134, 139, 182
R/pdt01.R                       61      38  37.70%   35-52, 108-150
R/utils.R                      127      83  34.65%   70, 83-185
R/vst01.R                       48       0  100.00%
R/vst02.R                      102       3  97.06%   42, 120, 257
TOTAL                         2444     260  89.36%

Results for commit: a59b58cd25e8d4b5b001920d30848134260deacf

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

clarkliming
clarkliming reviewed Jan 5, 2023
View reviewed changes
R/egt03.R
#' where no name is provided, the label attribute of the corresponding column in `adeg` table of `adam_db` is used.
#'
#' @details
#' * the number of patients by baseline assessment and minimum post-baseline assessment.
Copy link
Contributor

Choose a reason for hiding this comment

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

adeg data are subsetted to contain only "Postbaseline maximum" or "postbaseline minimum" visit, this should be included in details, and also checked in pre processing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks liming. now fixed

clarkliming
clarkliming reviewed Jan 5, 2023
View reviewed changes
R/egt03.R
) %>%
mutate(min_label = "Minimum Post-Baseline Assessment") %>%
mutate(BNRIND = factor(
BNRIND,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have the possibility that "Missing" is not included in the result. e.g, if it is already a factor, we do not add new levels again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was referring the tlg-c, I think it was added before to ensure the columns always have 4 levels/columns. No strong opinion for this. I was wondering if @khatril and @barnett11 can weigh in here? Thanks

clarkliming
clarkliming reviewed Jan 11, 2023
View reviewed changes
R/egt03.R Outdated
dm_zoom_to("adeg") %>%
filter(
PARAMCD == "HR" & # Heart Rate
SAFFL == "Y" & # "Safety Population Flag"
Copy link
Contributor

Choose a reason for hiding this comment

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

this SAFFL should not be used as it is filter related

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 we need to assert this?

Copy link
Contributor

Choose a reason for hiding this comment

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

no need to assert this; it should not be handled by the template

clarkliming
clarkliming reviewed Jan 11, 2023
View reviewed changes
R/egt03.R Show resolved Hide resolved
shajoezhu
shajoezhu commented Jan 12, 2023
View reviewed changes
R/egt03.R Outdated Show resolved Hide resolved
@shajoezhu
Update R/egt03.R， remove filter
22a76a8 
Signed-off-by: Joe Zhu <sha.joe.zhu@gmail.com>
clarkliming
clarkliming approved these changes Jan 12, 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.

I think we can merge this and leave the rest test in other issues

@BFalquet
Copy link
Contributor

R/egt03.R 129 85 34.11% 32-52, 111, 125-232, 291, 305-339

Could it be possible to slightly increase test coverage ? (can be done in another issue). Thank you for this great template.

@shajoezhu
Copy link
Contributor Author

Cool! Thanks @clarkliming

@shajoezhu shajoezhu merged commit 1993446 into main Jan 12, 2023
@shajoezhu shajoezhu deleted the 254_egt03 branch January 12, 2023 08:29
@shajoezhu shajoezhu mentioned this pull request Jan 12, 2023
Closed
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

@barnett11 barnett11 Awaiting requested review from barnett11

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@shajoezhu @BFalquet @clarkliming