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

adapt to reformat function in dunlin #409

Merged
merged 7 commits into from
Mar 8, 2023
Merged

adapt to reformat function in dunlin #409

merged 7 commits into from
Mar 8, 2023

Conversation

BFalquet
Copy link
Contributor

@BFalquet BFalquet commented Feb 24, 2023

adapt chevron preprocessing and tests to the new reformat method from dunlin.
Sister PR to the dunlin PR: insightsengineering/dunlin#58 please merge at the same time.

thank you for the review

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2023

🧪 $Test coverage: 91.60%$

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ------------------------------------------------------------------------
R/aet01_aesi.R                 187       8  95.72%   37, 39-43, 266, 278
R/aet01.R                      328       6  98.17%   36, 238, 250, 312, 318, 544
R/aet02.R                      219       2  99.09%   129, 469
R/aet03.R                       81       0  100.00%
R/aet04.R                      105       2  98.10%   162, 167
R/assertions.R                  64       0  100.00%
R/checks.R                      20       0  100.00%
R/chevron_tlg-S4class.R         21       0  100.00%
R/chevron_tlg-S4methods.R      131      13  90.08%   43, 408-480
R/cmt01a.R                     189       0  100.00%
R/cmt02_pt.R                    51       0  100.00%
R/dmt01.R                       31       0  100.00%
R/dst01.R                      292       0  100.00%
R/dtht01.R                      96       0  100.00%
R/egt01.R                       45       0  100.00%
R/egt02.R                       58       0  100.00%
R/egt03.R                      130       3  97.69%   114, 158, 311
R/egt05_qtcat.R                 55       0  100.00%
R/ext01.R                       79       8  89.87%   230-233, 237-240
R/gen_args.R                     1       1  0.00%    29
R/lbt01.R                       94       0  100.00%
R/lbt04.R                       51       1  98.04%   120
R/lbt05.R                       74      10  86.49%   32-40, 145
R/lbt07.R                       86       1  98.84%   164
R/lbt14.R                      202      34  83.17%   53, 55, 105-108, 110-118, 148, 177, 259, 261, 311-314, 316-324, 354, 383
R/mht01.R                       71       2  97.18%   32-33
R/mng01.R                       93      12  87.10%   110, 114-117, 126-134, 177
R/pdt01.R                       60      37  38.33%   34-50, 102-142
R/pdt02.R                       67       0  100.00%
R/utils.R                      194     136  29.90%   70, 83-241, 402
R/vst01.R                       47       0  100.00%
R/vst02.R                      100       3  97.00%   40, 114, 245
TOTAL                         3322     279  91.60%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
R/aet03.R        -2       0  +100.00%
R/dmt01.R        -9       0  +100.00%
R/dst01.R        +9       0  +100.00%
R/dtht01.R       -6       0  +100.00%
R/lbt05.R        -1       0  -0.18%
R/lbt14.R        +2       0  +0.17%
R/utils.R        +6      +6  -0.95%
TOTAL            -1      +6  -0.18%

Results for commit: d6e6eb8858bb057a15305c686173c424d5a6558f

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2023

Unit Tests Summary

    1 files    26 suites   1m 43s ⏱️
129 tests   91 ✔️ 38 💤 0
277 runs  186 ✔️ 91 💤 0

Results for commit f3b19d6.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2023

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
cmt01a 💔 $3.14$ $+1.01$ $0$ $0$ $0$ $0$
default_tlg 💔 $38.93$ $+2.97$ $0$ $0$ $0$ $0$
dst01 💔 $5.65$ $+1.98$ $0$ $0$ $0$ $0$
empty_report 💔 $8.50$ $+1.93$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
empty_report 💔 $8.50$ $+1.93$ tlg_functions_return_null_reports_when_domain_table_is_empty

Results for commit d6e6eb8858bb057a15305c686173c424d5a6558f

♻️ This comment has been updated with latest results.

@@ -96,14 +96,14 @@ dmt01_1_pre <- function(adam_db) {

new_format <- list(
adsl = list(
SEX = list(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should contain this rule to change the display of SEX. this preprocessing is now displayed for users when they use script based approach, but don't know if it is so clear. @barnett11 what do you think?

R/lbt05.R Outdated
@@ -114,12 +113,12 @@ lbt05_1_pre <- function(adam_db, arm_var = "ACTARM") {

new_format <- list(
adlb = list(
AVALCAT1 = list("<Missing>" = c("", NA, "<Missing>", "No Coding Available")),
abn_dir = list("<Missing>" = c("", NA, "<Missing>", "No Coding Available"))
AVALCAT1 = rule("<Missing>" = c("", NA, "<Missing>", "No Coding Available")),
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe, we can add a missing rule to facilitate this, and remove the redudant rule definitions. anyway rule are desinged to be resued right

@BFalquet BFalquet requested a review from clarkliming March 7, 2023 18:07
@@ -482,15 +482,15 @@ aet01_2_pre <- function(adam_db) {
) %>%
dm_update_zoomed()

missing_list <- list("<Missing>" = c("", NA))
missing_rule <- rule("<Missing>" = c("", NA))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether these rules should be some static object, instead of being defined on the fly? (but anyway these rules are super fast probably we keep as is).

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.

very minor comment on whether we should store these rules, like missing_rule = rule("<Missing>" = c("", NA_character)) somewhere but it should be fine for now

or open a issue for further discussion @BFalquet

@clarkliming clarkliming merged commit ded40c0 into main Mar 8, 2023
@clarkliming clarkliming deleted the 181_reformat@main branch March 8, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants