-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Update NA handling by s_summary
#924
Conversation
Code Coverage Summary
Diff against main
Results for commit: 0b68fb5 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
i wonder if we should leave this merge til the next PI. |
This is fine to leave until next PI. It's not urgent since everything is working as is - just a nice improvement (in my opinion). |
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.
Lgtm! I do not think we need to wait for next PI. I am just wondering if all the rest of tern behaves correctly, i.e. uses na_level
only for replacement and not for na.rm
. Also, have you checked if this affects scda.test
in any way?
There are no templates/tests that use this argument as a default, so there is no change in |
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.
LGTM! Great work Emily :)
Since
s_compare
is a wrapper fors_summary
it has also been updated accordingly.Updates:
NA
values in input variablesNA
s, ifna.rm = FALSE
an explicitNA
level will be automatically addedna.rm = TRUE
,<Missing>
will also be excluded - this way theNA
behaviour will work correctly whether or notdf_explicit_na
has already been applied (with default settings)na_level
NA
when settingna.rm = TRUE
NA
values in output tableCloses #898