123 DMT01 sample data review #321

BFalquet · 2022-11-15T15:30:35Z

close insightsengineering/chevron-tasks#53
close #322

change default sumlmarized values
add tests
adapt the synthetic data

thank you for the review

github-actions · 2022-11-15T15:41:24Z

🧪 $Test coverage: 94.09%$

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  -------------------------------
R/aet01.R                      335       6  98.21%   38, 241, 254, 317, 323, 559
R/aet02.R                      222       2  99.10%   136, 489
R/aet03.R                       85       0  100.00%
R/aet04.R                      105       6  94.29%   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/mht01.R                       72       2  97.22%   33-34
R/mng01.R                       93       9  90.32%   113, 117-120, 133-134, 139, 182
R/utils.R                      117      77  34.19%   70, 83-177
R/vst01.R                       48       0  100.00%
R/vst02.R                      102       3  97.06%   42, 120, 257
TOTAL                         2198     130  94.09%

Results for commit: 24601148da606faa1900e66668e36e0182edda4a

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2022-11-15T15:44:45Z

Unit Tests Summary

    1 files   15 suites 58s ⏱️
  80 tests   52 ✔️ 28 💤 0 ❌
173 runs 110 ✔️ 63 💤 0 ❌

Results for commit 729e991.

♻️ This comment has been updated with latest results.

clarkliming · 2022-11-16T06:12:17Z

R/utils.R

@@ -99,6 +99,10 @@ syn_test_data <- function() {
    )
  )
  sd$adex <- dunlin::cut_by_group(as.data.frame(sd$adex), "AVAL", "PARAM", group, "AVALCAT1")
+  sd$adsl$AAGE <- sd$adsl$AGE
+  attr(sd$adsl$AAGE, "label") <- "Age (yr)"
+  sd$adsl$AGEGR1 <- cut(sd$adsl$AGE, c(0, 18, 200), c("<18", ">=18"))


we can change the default data to contain age group <65 and >= 65

good idea. Done!

barnett11

I still feel we should provide labels for AAGE and AGEGR1 as the user will always want to overwrite from default adsl labels,
run(dmt01_1, adam_db = db, armvar="TRT01P", summaryvars = c("AAGE", "AGEGR1"), summaryvars_lbls= c("Age (yr)","Age group (yr)"))

BFalquet · 2022-11-16T16:23:07Z

I still feel we should provide labels for AAGE and AGEGR1 as the user will always want to overwrite from default adsl labels, run(dmt01_1, adam_db = db, armvar="TRT01P", summaryvars = c("AAGE", "AGEGR1"), summaryvars_lbls= c("Age (yr)","Age group (yr)"))

@barnett11 thank you for the review. Default labels have been updated to reflect standard labels.

barnett11

Looking great, thanks @BFalquet

clarkliming · 2022-11-17T11:57:25Z

R/dmt01.R

-                         summaryvars = c("AGE", "SEX", "COUNTRY", "RACE"),
-                         summaryvars_lbls = NULL,
+                         summaryvars = c("AAGE", "AGEGR1", "SEX", "ETHNIC", "RACE"),
+                         summaryvars_lbls = c("Age (yr)", "Age group (yr)", "Sex", "Ethnicity", "Race"),


I don't agree that we should provide these labels here. The thing is, if labels are provided as default, then when you supply the summaryvars other than AAGE and AGEGR1, you have to update the summaryvars_lbls otherwise there will be issues.

example:

run(dmt01_1, syn_data, summaryvars = "AAGE")
Error in (function (adam_db, armvar = "ARM", summaryvars = c("AAGE", "AGEGR1", :
Assertion on 'length(summaryvars) == length(summaryvars_lbls)' failed: Must be TRUE.

it just looks not good.

@clarkliming I understand the objection, what about a named vector then ? e.g. summaryvars = c("Age (yo)" = "AGE") and for the unnamed elements we collect the label from the data ? Or the summaryvars _lbls could be a named vector with the same logic (but then it would be a bit redundant) summaryvars = c( "AGE"), summaryvars _lbls = c("Age (yo)" = "AGE").

After discussion with @clarkliming and @barnett11 the first option has been selected for implementation.

clarkliming · 2022-11-17T14:55:43Z

R/utils.R

+  summaryvars_lbls <- c()
+
+  for (i in seq_along(x)) {
+    res <- if (is.null(names(x[i])) || names(x[i]) == "") {


I think we can vectorize it; like

if (is.null(names(x)) {labels} else {if_else(names(x)== "", labels, names(x)}

of course names(x) should be executed outside

clarkliming

looks good to me!

benoit added 2 commits November 15, 2022 15:32

add test

1bbc35e

adapt syn_data

5d69b92

BFalquet requested a review from barnett11 November 15, 2022 17:02

clarkliming reviewed Nov 16, 2022

View reviewed changes

change AGEGP1 and adapt tests

5469343

barnett11 requested changes Nov 16, 2022

View reviewed changes

update default labels

6878d83

BFalquet requested a review from barnett11 November 16, 2022 16:26

adapt test

9cbce39

barnett11 approved these changes Nov 17, 2022

View reviewed changes

clarkliming reviewed Nov 17, 2022

View reviewed changes

summaryvars as dictionnary

a9d7253

clarkliming reviewed Nov 17, 2022

View reviewed changes

vectorize

729e991

clarkliming approved these changes Nov 18, 2022

View reviewed changes

clarkliming merged commit 40c5bfd into main Nov 18, 2022

clarkliming deleted the 123-dmt01-sample-data-review@main branch November 18, 2022 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

123 DMT01 sample data review #321

123 DMT01 sample data review #321

BFalquet commented Nov 15, 2022 •

edited

Loading

github-actions bot commented Nov 15, 2022 •

edited

Loading

github-actions bot commented Nov 15, 2022 •

edited

Loading

clarkliming Nov 16, 2022

BFalquet Nov 16, 2022

barnett11 left a comment

BFalquet commented Nov 16, 2022 •

edited

Loading

barnett11 left a comment

clarkliming Nov 17, 2022

clarkliming Nov 17, 2022

clarkliming Nov 17, 2022

BFalquet Nov 17, 2022

BFalquet Nov 17, 2022

clarkliming Nov 17, 2022

clarkliming left a comment

123 DMT01 sample data review #321

123 DMT01 sample data review #321

Conversation

BFalquet commented Nov 15, 2022 • edited Loading

github-actions bot commented Nov 15, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Nov 15, 2022 • edited Loading

Unit Tests Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barnett11 left a comment

Choose a reason for hiding this comment

BFalquet commented Nov 16, 2022 • edited Loading

barnett11 left a 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 left a comment

Choose a reason for hiding this comment

BFalquet commented Nov 15, 2022 •

edited

Loading

github-actions bot commented Nov 15, 2022 •

edited

Loading

github-actions bot commented Nov 15, 2022 •

edited

Loading

BFalquet commented Nov 16, 2022 •

edited

Loading