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.

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

PDT01 #328

Merged
merged 15 commits into from
Dec 7, 2022
Merged

PDT01 #328

merged 15 commits into from
Dec 7, 2022

Conversation

shajoezhu
Copy link
Contributor

@shajoezhu shajoezhu commented Nov 23, 2022

close #244

@shajoezhu shajoezhu added the sme label Nov 23, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 23, 2022

🧪 $Test coverage: 92.44%$

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/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                         2315     175  92.44%

Results for commit: 31896a9f3c914806028bdf26e37e247396492749

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Nov 23, 2022

Unit Tests Summary

    1 files    17 suites   1m 9s ⏱️
  85 tests   56 ✔️ 29 💤 0
182 runs  114 ✔️ 68 💤 0

Results for commit 9e5b85a.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

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

Thanks @shajoezhu , some initial suggestions,

  1. should template have _1 appended in same naming convention as other templates?
  2. The column headers should read Category and Description rather than the variable labels from the data
    image

@shajoezhu
Copy link
Contributor Author

Hi @barnett11 , thanks for the review. Now fixed. Can you take another look please? Many thanks

image

@shajoezhu shajoezhu requested a review from barnett11 November 29, 2022 00:49
Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

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

Percentage format to be xx.x, even with eg. 42.0%

@clarkliming
Copy link
Contributor

clarkliming commented Nov 30, 2022

Hi @shajoezhu we can another format in count_occurence, maybe you can add it in tern package?

f <- function(x) {
  if (x[1] == 0) {
    return("0")
  } else {
    sprintf("%d (%.1f%%)", x[1], x[1] / x[2] * 100)
  }
}

f(c(0,10))

f(c(1,10))

@shajoezhu shajoezhu requested a review from barnett11 November 30, 2022 08:51
@shajoezhu
Copy link
Contributor Author

Thanks @barnett11 and @clarkliming , I have updated the template, I am going to raise a new issue in formatters, and we can do an enanced version of the formatting "xx (xx.x%)", and to display "0", when xx is zero

Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

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

Hi @shajoezhu - I still get the %dp issue in the top row,
image

@shajoezhu shajoezhu requested a review from barnett11 December 1, 2022 18:53
@clarkliming
Copy link
Contributor

Hi @barnett11 is this ready to be merged?

Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

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

Hi @shajoezhu
The percentage formatting is working for the lower categories, but the top level is not impacted (Total number of patients with at least one major protocol deviation) by this change

update use format_count_fraction_fixed_dp

Signed-off-by: Joe Zhu <[email protected]>
@shajoezhu
Copy link
Contributor Author

Thanks @barnett11 , just made another push. I think we should do a global change in chevron, and apply the tern format tern::format_count_fraction_fixed_dp https://github.com/insightsengineering/tern/blob/bb0a914d9525507d381d6be1fd4c5eca272f6226/R/formats.R#L134

Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

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

Beautiful thanks @shajoezhu

@shajoezhu shajoezhu merged commit 0d2ce71 into main Dec 7, 2022
@shajoezhu shajoezhu deleted the 244_pdt01@main branch December 7, 2022 09:19
@shajoezhu
Copy link
Contributor Author

Thanks @barnett11 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDT01(Spec added)
3 participants