-
-
Notifications
You must be signed in to change notification settings - Fork 15
add option to remove empty columns in tm_t_crosstable #890
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 Summary
Diff against main
Results for commit: 6f89792 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 22 suites 2s ⏱️ Results for commit 8dba181. |
Unit Tests Summary 1 files 22 suites 1s ⏱️ Results for commit 6f89792. ♻️ 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.
I thought we would automatically drop the columns with 0 observations, but I think it makes sense to include this as an encoding option.
Everything works as intended, however, show_percentage
and show_total
are arguments for this function and are part of the Table Settings element. Since we're adding remove_zero_columns
to the Table Settings, I think it should also be an argument to keep it consistent with show_percentage
and show_total
.
@kumamiao would you agree?
Agree to add argument to be consistent across |
Hey, added a parameter data <- teal_data()
data <- within(data, {
mtcars <- mtcars
for (v in c("cyl", "vs", "am", "gear")) {
mtcars[[v]] <- as.factor(mtcars[[v]])
}
mtcars[["primary_key"]] <- seq_len(nrow(mtcars))
})
join_keys(data) <- join_keys(join_key("mtcars", "mtcars", "primary_key"))
app <- init(
data = data,
modules = modules(
tm_t_crosstable(
label = "Cross Table",
x = data_extract_spec(
dataname = "mtcars",
select = select_spec(
label = "Select variable:",
choices = variable_choices(data[["mtcars"]], c("cyl", "vs", "am", "gear")),
selected = c("cyl", "gear"),
multiple = TRUE,
ordered = TRUE,
fixed = FALSE
)
),
y = data_extract_spec(
dataname = "mtcars",
select = select_spec(
label = "Select variable:",
choices = variable_choices(data[["mtcars"]], c("cyl", "vs", "am", "gear")),
selected = "vs",
multiple = FALSE,
fixed = FALSE
)
),
remove_zero_columns = TRUE
)
)
)
if (interactive()) {
shinyApp(app$ui, app$server)
} |
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.
Pull Request Overview
This PR adds a new remove_zero_columns
option to the cross-table module, allowing users to drop columns that contain only zeros.
- Introduces
remove_zero_columns
parameter in both UI and server - Updates documentation (Rd and roxygen) with the new option
- Implements conditional logic to drop empty columns before building the table
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
man/tm_t_crosstable.Rd | Adds “Remove zero-only columns” under the Table Settings section |
R/tm_t_crosstable.R | Introduces remove_zero_columns argument, UI checkbox, server logic, and branching behavior |
Comments suppressed due to low confidence (1)
R/tm_t_crosstable.R:169
- The new
remove_zero_columns
option needs accompanying unit and integration tests to verify that empty columns are correctly removed when enabled and retained when disabled.
remove_zero_columns = FALSE,
@kumamiao @donyunardi did you have chance to review? |
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.
Please add entry to NEWS.md before merging.
Closes https://github.com/insightsengineering/coredev-tasks/issues/363