253 egt05 qtcat@main #346

JiaLiu0001 · 2022-12-29T07:28:02Z

close insightsengineering/chevron-tasks#50

github-actions · 2022-12-29T07:38:44Z

🧪 $Test coverage: 87.96%$

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ------------------------------------------------------------------------
R/aet01_aesi.R                 188       8  95.74%   39, 41-45, 272, 285
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                       84       0  100.00%
R/aet04.R                       93       0  100.00%
R/assertions.R                  36       0  100.00%
R/checks.R                      20       0  100.00%
R/chevron_tlg-S4class.R         21       0  100.00%
R/chevron_tlg-S4methods.R      123      17  86.18%   35, 94-102, 137-145, 396-468
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                      132      91  31.06%   32-52, 120-243, 290-326
R/egt05_qtcat.R                 57      33  42.11%   38-57, 119-142
R/ext01.R                       81       8  90.12%   238-241, 245-248
R/gen_args.R                     1       1  0.00%    30
R/lbt01.R                       95       0  100.00%
R/lbt04.R                       52       1  98.08%   127
R/lbt05.R                       76      10  86.84%   33-41, 152
R/lbt07.R                       81       2  97.53%   33, 163
R/lbt14.R                      202      34  83.17%   57, 59, 107-110, 112-120, 150, 181, 268, 270, 318-321, 323-331, 361, 392
R/mht01.R                       72       2  97.22%   33-34
R/mng01.R                       93      12  87.10%   113, 117-120, 129-137, 182
R/pdt01.R                       61      38  37.70%   35-52, 108-150
R/pdt02.R                       68       0  100.00%
R/utils.R                      188     130  30.85%   70, 83-235, 396
R/vst01.R                       48       0  100.00%
R/vst02.R                      102       3  97.06%   42, 120, 257
TOTAL                         3306     398  87.96%

Results for commit: 9ba6e1b51364000d41e0fbd78c1f372d1e790dc8

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2022-12-29T07:39:36Z

Unit Tests Summary

    1 files   26 suites 1m 27s ⏱️
121 tests   86 ✔️ 35 💤 0 ❌
270 runs 182 ✔️ 88 💤 0 ❌

Results for commit e5233d5.

♻️ This comment has been updated with latest results.

R/egt05_qtcat.R

clarkliming · 2023-01-04T03:31:46Z

R/egt05_qtcat.R

+      PARAMCD == "QT"
+      ) %>%
+    mutate(
+      AVALCAT1 = case_when(


please consider only derive the AVALCAT1 if there is no variable available? if it is already derived, then I think it is not necessary to derived it again here

and unit should come from AVALU? although the chance that avalu is not "msec" is small

@JiaLiu0001 is AVALU considered here? whether we should use AVALU as the unit, instead of "msec"?

clarkliming · 2023-01-04T05:26:54Z

R/egt05_qtcat.R

+#'
+#' @note
+#'  * `adam_db` object must contain an `adeg` table with a `"PARAM"` column contains 'QT' as well as columns
+#'  specified in `summaryvars` and `visitvar`.


please consider making the algorithm clear: visit var is needed in adeg, while summaryvars does not. it is derived in preprocess (at least for the default one)

clarkliming · 2023-01-04T05:27:24Z

R/egt05_qtcat.R

+egt05_qtcat_1_main <- function(adam_db,
+                               armvar = "ACTARM",
+                               summaryvars = c("Value at Visit" = "AVALCAT1", "Change from Baseline" = "CHGCAT1"),
+                               visitvar = "AVISIT", # or ATPTN


please remove the comment here

clarkliming · 2023-01-04T05:28:08Z

R/egt05_qtcat.R

+                              lbl_avisit,
+                              deco,
+                              ...) {
+  # TODE solve the problem of the overall column


please clean up comments here

clarkliming · 2023-01-04T08:30:53Z

R/egt05_qtcat.R

-          "<Missing>"
-          )
-        )
+      AVALCAT1 = factor(AVALCAT1),


this can lead to strange order factors

Thank you @clarkliming factor updated

clarkliming · 2023-01-05T05:02:45Z

R/egt05_qtcat.R

+#'  * `adam_db` object must contain an `adeg` table with a `"PARAM"` column contains 'QT' as well as column
+#'  specified in `visitvar`.
+#'  For `summaryvars`, if `AVALCAT1` and `CHGCAT1` columns are already existed in input data sets, no need to
+#'  specify `AVAL` or `CHG`. If not, `AVAL` and `CHG` columns must be contained.


no matther if AVALCAT1 exist, we don't specify "AVAL" or "CHG" in data; we only re-derive the "AVALCAT1" if it does not exist in data right?

sentence updated

clarkliming · 2023-01-05T05:04:57Z

R/egt05_qtcat.R

@@ -86,7 +92,8 @@ egt05_qtcat_1_lyt <- function(armvar,
      vars = summaryvars,
      var_labels = summaryvars_lbls
    ) %>%
-    append_topleft("  Category")
+    append_topleft("Analysis Visit") %>%


please use an argument instead of hard coding

hard coding updated

clarkliming · 2023-01-06T06:33:46Z

I think it looks good now, but let's wait for @barnett11 for a final review

R/egt05_qtcat.R

clarkliming · 2023-01-11T14:45:31Z

R/egt05_qtcat.R

+#'
+egt05_qtcat_1_pre <- function(adam_db, paramcdvar = "QT", ...) {
+  checkmate::assert_class(adam_db, "dm")
+  unit <- adam_db$adeg %>% filter(PARAMCD == paramcdvar) %>% select(AVALU) %>% unique()


please follow style guide that after %>% always start a new line; in addition, we also need to check unit is length 1 otherwise there will be issues in follow up code; in addition we can use pull instead of select to get the value

@JiaLiu0001 how is this going?

The update is not pushed yet. I'm wondering 'do we need different cut-offs for "QT", "QTCF", etc' like you mentioned in the above comment? If so, I prefer to update them in one push. @clarkliming

let's update the formats and merge; for the testing part let's track in another issue and Tim will take over then

clarkliming · 2023-01-11T14:46:48Z

R/egt05_qtcat.R

+        AVAL <= 450 ~ paste0("<=450 ", unit),
+        AVAL <= 480 ~ paste0(">450 to <=480 ", unit),
+        AVAL <= 500 ~ paste0(">480 to <= 500 ", unit),
+        AVAL > 500 ~ paste0(">500 ", unit),


do we need different cut-offs for "QT", "QTCF", etc? @barnett11

These thresholds should not be derived in the TLG code, they should come directly from AVALCAT1 and CHGCAT1

ok @JiaLiu0001 maybe we just remove this preprocessing, and add AVALCAT1 and CHGCAT1 in data to make sure the example works

Thanks @clarkliming
Pre-defines of AVALCAT1 and CHGCAT1 are removed, and warnings will return if AVALCAT1 and CHGCAT1 do not exist. However, I'm not sure how to update the data here to add AVALCAT1 and CHGCAT1 to the dataset. Could you please help with this?

in get_syn_data function, add appropriate derivations of AVALCAT1 and CHGCAT1. In addition, we should give errors instead of warnings when the variables do not exist

Hi @clarkliming I didn't find the function get_syn_data in https://github.com/insightsengineering. Could you please point me to the function you mentioned?

clarkliming · 2023-02-06T06:37:54Z

DESCRIPTION

@@ -54,7 +54,7 @@ Config/testthat/edition: 3
 Encoding: UTF-8
 LazyData: true
 Roxygen: list(markdown = TRUE)
-RoxygenNote: 7.2.3
+RoxygenNote: 7.2.3.9000


are you using a develop version of Roxygen?

Yes, I'm. Looks like it should be switched to 7.2.3?

clarkliming · 2023-02-06T07:04:43Z

R/egt05_qtcat.R

+#' @param ... not used.
+#'
+#' @export
+egt05_qtcat_1_lyt <- function(armvar,


please double check if the arguments of egt05_qtcat_1_lyt are also exposed in egt05_qtcat_1_main

clarkliming · 2023-02-06T07:09:30Z

R/egt05_qtcat.R

+    ) %>%
+    mutate(
+      AVALCAT1 = if ("AVALCAT1" %in% colnames(.)) factor(AVALCAT1) else
+        stop("Please make sure 'AVALCAT1' is existed")


please use assert_colnames to check if variables are valid

clarkliming · 2023-02-07T13:51:19Z

@JiaLiu0001 are you still working on this?

add AVALCAT1 and CHGCAT1 into test data.

JiaLiu0001 · 2023-02-08T08:58:04Z

@JiaLiu0001 are you still working on this?
Hi @clarkliming, Once I replaced fct_explicit_na by tern::explicit_na in utils.R, looks like in CMD check, ext01_2 has been impected. Should I just recover this fct_explicit_na change?

clarkliming · 2023-02-09T01:28:45Z

@JiaLiu0001 are you still working on this?
Hi @clarkliming, Once I replaced fct_explicit_na by tern::explicit_na in utils.R, looks like in CMD check, ext01_2 has been impected. Should I just recover this fct_explicit_na change?

Also convert the variable to factor

JiaLiu0001 · 2023-02-09T04:47:57Z

@JiaLiu0001 are you still working on this?
Hi @clarkliming, Once I replaced fct_explicit_na by tern::explicit_na in utils.R, looks like in CMD check, ext01_2 has been impected. Should I just recover this fct_explicit_na change?

Also convert the variable to factor

Hi @clarkliming Even though fct_explicit_na is replaced by explicit_na, still got Warning in CMD. Do you have any idea about it?

clarkliming · 2023-02-09T04:49:38Z

it is in dunlin and I have open issues to address it

…ineering/chevron into 253_EGT05_QTCAT@main

clarkliming · 2023-02-10T05:27:03Z

R/utils.R

+  # Add AVALCAT1 CHGCAT1 for adeg
+  sd$adeg <- sd$adeg %>%
+    mutate(
+      AVALCAT1 = if ("AVALCAT1" %in% colnames(.)) {


in synthetic_data, whether AVALCAT1 exists has a clear answer, yes or no. No need to check if it exists. It is not user input.

For baseline visits, where CHG is NA, please do not derive values; Do not keep "NA msec" here

Please derive CHGCAT /AVALCAT only for QT (for now)

Thank you @clarkliming, the utils is updated as your comments

Thank you @clarkliming, the utils is updated as your comments

Hi @clarkliming Have this PR passed review?

clarkliming

looks good to me; please resolve the conflict and merge.

After merged, you can open a new issue and tag Tim for a final review

Merge branch 'main' into 253_EGT05_QTCAT@main # Conflicts: # R/utils.R

JiaLiu0001 added 3 commits December 29, 2022 05:01

setup egt05_qtcat

61fb307

add test-egt05_qtcat

911125b

update NAMESPACE and Rd

fad1f53

JiaLiu0001 added 4 commits December 29, 2022 08:09

update to pass superlinter

f32edda

update the annotation

99de9e7

update man/Rd for annotations

5c3d537

update wordlist

5a4ce1e

clarkliming reviewed Jan 4, 2023

View reviewed changes

R/egt05_qtcat.R Show resolved Hide resolved

clarkliming reviewed Jan 4, 2023

View reviewed changes

R/egt05_qtcat.R Outdated Show resolved Hide resolved

clarkliming reviewed Jan 4, 2023

View reviewed changes

update according to the comments

0d01cb7

clarkliming reviewed Jan 4, 2023

View reviewed changes

clarkliming reviewed Jan 5, 2023

View reviewed changes

update factor issue and hard coding

6cbf3d1

edelarua added the sme label Jan 6, 2023

barnett11 self-requested a review January 10, 2023 13:54

barnett11 requested changes Jan 10, 2023

View reviewed changes

R/egt05_qtcat.R Outdated Show resolved Hide resolved

JiaLiu0001 added 3 commits January 11, 2023 10:40

update for AVALU and PARAMCD

e1b52fd

spell check pass

02b453c

update wordlist

d13b0a9

clarkliming reviewed Jan 11, 2023

View reviewed changes

Remove pre-defined CHGCAT1 and AVALCAT1 from preprocessing

c332141

clarkliming reviewed Feb 6, 2023

View reviewed changes

clarkliming mentioned this pull request Feb 7, 2023

egt05_qtcat empty table test #387

Closed

JiaLiu0001 added 3 commits February 8, 2023 06:26

make multiple PARAMCD selections available.

ef3cc71

add AVALCAT1 and CHGCAT1 into test data.

Pass lintr

6532a20

replace fct_explicit_na with tern::explicit_na

4ca7b5b

explicit_na add factor

e1d3cae

clarkliming mentioned this pull request Feb 9, 2023

resolve warning related to forcats::fct_explicit_na insightsengineering/dunlin#49

Closed

shajoezhu and others added 5 commits February 9, 2023 13:13

Merge branch 'main' into 253_EGT05_QTCAT@main

3217dcb

fix style check

c1149b6

Merge branch '253_EGT05_QTCAT@main' of https://github.com/insightseng…

a68ae91

…ineering/chevron into 253_EGT05_QTCAT@main

pass roxygen check

c44131d

update arm_var

a1ac2c3

clarkliming reviewed Feb 10, 2023

View reviewed changes

JiaLiu0001 added 2 commits February 10, 2023 06:42

update AVALCAT1 CHGCAT1 in utils

d977dc2

pass style

519529b

clarkliming approved these changes Feb 14, 2023

View reviewed changes

barnett11 approved these changes Feb 16, 2023

View reviewed changes

JiaLiu0001 added 4 commits February 16, 2023 09:03

merge main

6bcac07

Merge branch 'main' into 253_EGT05_QTCAT@main # Conflicts: # R/utils.R

roxygen check

fe42920

make vignette title and yaml.meta title unify

c9cb5bd

recover fuse

e5233d5

JiaLiu0001 merged commit 9a8ee91 into main Feb 16, 2023

JiaLiu0001 deleted the 253_EGT05_QTCAT@main branch February 16, 2023 10:29

253 egt05 qtcat@main #346

253 egt05 qtcat@main #346

Conversation

JiaLiu0001 commented Dec 29, 2022

github-actions bot commented Dec 29, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Dec 29, 2022 • edited Loading

Unit Tests Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarkliming commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarkliming commented Feb 7, 2023

JiaLiu0001 commented Feb 8, 2023

clarkliming commented Feb 9, 2023

JiaLiu0001 commented Feb 9, 2023

clarkliming commented Feb 9, 2023

Choose a reason for hiding this comment

JiaLiu0001 Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarkliming left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2022 •

edited

Loading

github-actions bot commented Dec 29, 2022 •

edited

Loading

JiaLiu0001 Feb 10, 2023 •

edited

Loading