-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
issue with pre functions #491
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Signed-off-by: Liming <[email protected]>
🧪 Code Coverage Summary
Diff against main
Results for commit: 19e2725d16d02bca953b402c0a49be7f637daa10 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor point inside (could be solved later). Thank you for the hard work
tests/testthat/_snaps/aet01_aesi.md
Outdated
@@ -13,7 +13,8 @@ | |||
Grade 2 6 (4.5%) 10 (7.5%) 7 (5.3%) | |||
Grade 3 18 (13.4%) 14 (10.4%) 16 (12.1%) | |||
Grade 4 15 (11.2%) 20 (14.9%) 18 (13.6%) | |||
Grade 5 (fatal outcome) 76 (56.7%) 70 (52.2%) 75 (56.8%) | |||
Grade 5 76 (56.7%) 70 (52.2%) 75 (56.8%) | |||
Missing 0 0 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if missing should appear (I dont think so). What do you think @barnett11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a difficult issue. We can expect Missing
grade in adae.atoxgr, (even in cleaned adam). We should not filter this out because we also need to count the number of events. But this should not be counted in the count_occurrences_by_grade
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. for ATOXGR
, those records are still using NA
(not convertet to a level) and then will be discarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another update: ADAE.ATOXGR is a string (character
in R's language). in rtables we require ATOXGR to be factor
. Then we need to convert it into factor.
so here while we apply reformating, we need to populate NA
for empty strings. Very different logic here!
reformat(c('1','2','3','4','5',''), rule('1'='1','2'='2','3'='3','4'='4','5'='5'))
my suggestion is to keep this missing level for now (as this solves many issues) and open another issue to deal with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably need to evaluate the reformat
when it is partial match of the whole set, option 1, keep the rest, option 2, discard the rest levels (use NA to replace) (but this is really contradictory to what we are doing to conver NA to "Missing"!) The logic/design behind this need to be further evaluated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue opened here: #492
Co-authored-by: b_falquet <[email protected]> Signed-off-by: Liming <[email protected]>
Good to merge from my side @clarkliming, issue has been opened for the reaming details |
I further adapt the code to remove the missing levels. hope this time it works |
export checks and use
::
in pre functionspart of #476