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

try to harmonize the templates #496

Merged
merged 75 commits into from
May 17, 2023
Merged

Conversation

clarkliming
Copy link
Contributor

@clarkliming clarkliming commented May 5, 2023

following #476, I tried to make the templates a bit clearer. something added

  1. for aet01_aesi and aet01 template, remove the check functions in pre. all these derivations are executed without check! as users are responsible for these derivations and they are clear. more checks added in main functions
    1. check the arm variable are identical in levels (in adsl and adae)
    2. check variables to analyze exists (especially those new derived)
    3. remove deco. titles/footnotes should be handled in citril using lopo
  2. remove aet01_2. this can be handled through arguments of aet01
  3. for aet02 template, variants are removed. can be achieved through argument row_split_var
  4. add many utility functions (currently put in aet02.R but should go into utility.R)
    3. valid_row_path. this checks if a row_path is valid for a table
    4. valid_sort_at_path. this conduct the sort based on row_path if it is valid. (sort_at_path gives error on incorrect row_path provided, and aet02_1 previously will run into issues if prune_0 = FALSE on empty data)
    5. get_sort_path. this function will create "" between variables, e.g, get_sort_path(c("A","B")) will give "A" "" "B", this is required by sort_at_path
    6. tlg_sort_by_var. this functions will sort using "A" and "B", instead of "A" "*" "B". Also will check if the table contains the path. if does not contain the path, the sorting will be ignored.

I think some of the funcitonality should go into tern/rtables. But for now let's use them.

Please review this update in general and I will finalize this PR if no objections. Then everyone can collaborate to harmonize all templates.

@Teninq @BFalquet @duanx9 @crazycatandy @barnett11

@clarkliming clarkliming requested review from Teninq, BFalquet and duanx9 May 5, 2023 07:23
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2023

🧪 $Test coverage: 95.99%$

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  --------------------------------
R/ael01_nollt.R                 17       1  94.12%   61
R/aet01_aesi.R                 147       1  99.32%   209
R/aet01.R                       92       1  98.91%   156
R/aet02.R                       57       0  100.00%
R/aet03.R                       76       0  100.00%
R/aet04.R                       88       0  100.00%
R/aet10.R                       42       0  100.00%
R/assertions.R                 146      22  84.93%   41-42, 181-186, 307-313, 330-336
R/checks.R                      20       6  70.00%   70-76
R/chevron_tlg-S4class.R         21       0  100.00%
R/chevron_tlg-S4methods.R      112      12  89.29%   378-450
R/cmt01a.R                      76       9  88.16%   80-83, 141-145
R/coxt02.R                      40       1  97.50%   112
R/dmt01.R                       26       0  100.00%
R/dst01.R                       93       0  100.00%
R/dtht01.R                     102       6  94.12%   48, 52-56
R/egt01.R                      102       0  100.00%
R/egt02.R                       35       0  100.00%
R/egt03.R                       61       1  98.36%   128
R/egt05_qtcat.R                 59       0  100.00%
R/ext01.R                       46       4  91.30%   110-111, 115-116
R/kmg01.R                       28       1  96.43%   69
R/lbt01.R                      100       0  100.00%
R/lbt04.R                       69       0  100.00%
R/lbt05.R                       66       5  92.42%   126-131
R/lbt07.R                       93       0  100.00%
R/lbt14.R                       92      10  89.13%   97-98, 100-106, 141
R/mht01.R                       71       0  100.00%
R/mng01.R                      109      12  88.99%   128, 132-135, 144-152, 192
R/pdt01.R                       60       0  100.00%
R/pdt02.R                       67       0  100.00%
R/rmpt01.R                      46       2  95.65%   90, 111
R/rspt01.R                      74       3  95.95%   160-163
R/rtables_utils.R              161       2  98.76%   41, 98
R/utils.R                       50       8  84.00%   136, 149-153, 162-170
R/vst01.R                      105       0  100.00%
R/vst02.R                       46       1  97.83%   106
TOTAL                         2695     108  95.99%

Diff against main

Filename                     Stmts    Miss  Cover
-------------------------  -------  ------  --------
R/ael01_nollt.R                 -9      -1  +1.81%
R/aet01_aesi.R                 -42      -6  +3.02%
R/aet01.R                     -250      -4  +0.38%
R/aet02.R                     -178      -1  +0.43%
R/aet03.R                       -7       0  +100.00%
R/aet04.R                      -18      -2  +1.89%
R/aet10.R                       -8      -1  +2.00%
R/assertions.R                 +75     +17  -8.03%
R/checks.R                       0      +6  -30.00%
R/chevron_tlg-S4methods.R      -10       0  -0.88%
R/cmt01a.R                    -110      +9  -11.84%
R/coxt02.R                     -13       0  -0.61%
R/dmt01.R                       -4       0  +100.00%
R/dst01.R                     -197       0  +100.00%
R/dtht01.R                      +4      +6  -5.88%
R/egt01.R                      +57       0  +100.00%
R/egt02.R                      -21       0  +100.00%
R/egt03.R                      -71      -2  +0.63%
R/egt05_qtcat.R                 +2       0  +100.00%
R/ext01.R                      -25       0  -3.06%
R/kmg01.R                       -9      -6  +15.35%
R/lbt01.R                       +6       0  +100.00%
R/lbt04.R                      +20       0  +100.00%
R/lbt05.R                       -2       0  -0.22%
R/lbt07.R                       +3      -1  +1.11%
R/lbt14.R                      -96     -14  +1.90%
R/mht01.R                       +2      -2  +2.90%
R/mng01.R                      +14       0  +1.62%
R/pdt01.R                       +3       0  +100.00%
R/pdt02.R                       +2       0  +100.00%
R/rmpt01.R                      -2       0  -0.18%
R/rspt01.R                     -12       0  -0.57%
R/rtables_utils.R             +161      +2  +98.76%
R/utils.R                      -33      +6  -13.59%
R/vst01.R                      +58       0  +100.00%
R/vst02.R                      -46      -1  +100.00%
TOTAL                         -756      +5  -1.06%

Results for commit: 348a473554d3cf185a252f92bca8a0c6e2dd1cb7

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2023

Unit Tests Summary

    1 files    39 suites   2m 6s ⏱️
166 tests 123 ✔️   43 💤 0
329 runs  225 ✔️ 104 💤 0

Results for commit 523fe1f.

♻️ This comment has been updated with latest results.

@clarkliming
Copy link
Contributor Author

clarkliming commented May 5, 2023

Hi @barnett11 , according to recent experience trying out chevron on -------, I tried to harmonize the templates following #476 . Could you have a look at this?

@clarkliming clarkliming requested a review from barnett11 May 5, 2023 15:02
@clarkliming
Copy link
Contributor Author

if no objections, I will also include https://github.com/insightsengineering/chevron.adr/issues/3 to remove unnecessary variants

@clarkliming
Copy link
Contributor Author

further updated aet02. there is one hacky update to check the data if it is empty, if empty returns the null report. this should be fixed. for the rest part I think we can use templates as example to update the rest

@clarkliming
Copy link
Contributor Author

the ae tables are updated. I will update read me in #476

Copy link
Contributor

@BFalquet BFalquet left a comment

Choose a reason for hiding this comment

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

I agree with the philosophy of these changes, but I am a bit concerned about the reaction of the users when their real data lead to an error or an unxepected result. There should be a general strategy to ensure data quality before entering the chevron pipline.

R/aet01_aesi.R Outdated Show resolved Hide resolved
R/aet03.R Outdated Show resolved Hide resolved
R/aet04.R Outdated Show resolved Hide resolved
R/ael01_nollt.R Outdated Show resolved Hide resolved
vignettes/chevron.Rmd Outdated Show resolved Hide resolved
vignettes/chevron.Rmd Outdated Show resolved Hide resolved
@clarkliming
Copy link
Contributor Author

clarkliming commented May 8, 2023

I agree with the philosophy of these changes, but I am a bit concerned about the reaction of the users when their real data lead to an error or an unxepected result. There should be a general strategy to ensure data quality before entering the chevron pipline.

how about we ensure that in pre functions we only use the dplyr/base R + reformat functionality? so that dplyr should still give informative errors. in main we use much stricter checks. @barnett11 what do you think?

basically, we have:

loose requirements in pre, as this is user facing, and this is really something like a template!

strong requirements in main, as users should not be able to touch inside this function! all input variables need to be checked!

@clarkliming clarkliming merged commit 5af2c1f into main May 17, 2023
@clarkliming clarkliming deleted the 476_harmonize_templates@main branch May 17, 2023 08:01
@shajoezhu
Copy link
Contributor

congratulations on the big merge!!!

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.

4 participants