-
-
Notifications
You must be signed in to change notification settings - Fork 15
Replace table by plot on tm_missing_data #922
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
Conversation
Code Coverage SummaryDiff against mainResults for commit: 01d9758 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 24 suites 15m 45s ⏱️ Results for commit 01d9758. ♻️ This comment has been updated with latest results. |
…g/teal.modules.general into 495_plot_missing@main
|
There are differences when tested with ADSL +ADRS data and when using just other data. The problem was mainly with the iris+mtcars used on the tests that require some more relaxed App with iris+mtcars (like tests)dev_mode()
devtools::install()
library("teal.modules.general")
source("tests/testthat/helper-TealAppDriver.R", echo = FALSE)
datat <- within(simple_teal_data(), {
set.seed(123)
add_nas <- function(x) {
x[sample(seq_along(x), floor(length(x) * runif(1, .05, .17)))] <- NA
x
}
iris[] <- lapply(iris, add_nas)
mtcars[] <- lapply(mtcars, add_nas)
mtcars[["cyl"]] <- as.factor(mtcars[["cyl"]])
mtcars[["gear"]] <- as.factor(mtcars[["gear"]])
})
teal::init(
data = datat,
modules = modules(tm_missing_data())
)|> runApp() |
averissimo
left a comment
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.
It seems ready to be merged.
I have 1 final question regarding borders to further separate the tiles.
Other comments are just technical details
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com>
llrs-roche
left a comment
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.
Thanks for the feedback. Applying the changes and suggesting news ones to keep the module coherent.
Signed-off-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com>
Signed-off-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com>
|
Thanks @llrs-roche and @averissimo for working on this PR. It looks good to me and I agree with replacing the table with the plot. Agree with Andre to display counts when counts is selected. |
…g/teal.modules.general into 495_plot_missing@main
averissimo
left a comment
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.
Awesome work @llrs-roche !! 💯


Pull Request
Fixes #495
Replaces the table by a plot on "By Variables Levels" tab.
The colors match those in the other plots of the module.
It also required to add a "new" dependency, formatters, which is a dependency of tern, rlistings, rtable. I added it on Suggests.
There is an extra commit to update the lintr names