Visualization: Collapse by [random] group#597
Conversation
Summary of ChangesHello @strengejacke, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new utility function, Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new function collapse_by_group for averaging raw data over random effect groups, which is a valuable addition for visualizing mixed models. The implementation is generally solid, but I have a few suggestions to improve its robustness and correctness. I've identified a high-severity issue with the handling of ordinal response variables that could lead to incorrect results, a medium-severity issue with brittle column selection logic, and a medium-severity issue with an uninformative and out-of-sync example in the documentation. Addressing these points will make the new function more reliable.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable new feature for visualizing mixed models by allowing data points to be collapsed by random effect groups. The implementation is mostly solid, with new tests, documentation, and vignette updates. However, I've identified two critical issues in the core logic that could lead to incorrect plots. My review includes specific suggestions to address these bugs and improve the robustness of the new functionality.
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to collapse/average data points by random effect grouping factors when plotting mixed models. It addresses issue #130 by implementing a new collapse_by_group() function and adding a collapse_group argument to plotting methods.
Changes:
- New
collapse_by_group()function to extract and average raw data points over random effect grouping factors for mixed models - Added
collapse_groupparameter toplot(),visualisation_recipe(), andtinyplot()methods for displaying collapsed data points - Updated documentation and vignette with examples showing how to use the collapsing feature
Reviewed changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| R/collapse_by_group.R | New function implementing data collapsing by random effects groups |
| R/visualisation_recipe.R | Added collapse_group parameter to visualization recipe methods |
| R/visualisation_recipe_internal.R | Added internal helper .visualization_recipe_collapsed_data() for rendering collapsed data |
| R/tinyplot.R | Integrated collapse_group parameter for tinyplot plotting method |
| man/collapse_by_group.Rd | Documentation for new function |
| man/visualisation_recipe.estimate_predicted.Rd | Updated documentation with new parameter |
| vignettes/plotting.Rmd | Added examples demonstrating the collapse feature |
| tests/testthat/test-plot-show_data.R | New tests for plotting with collapsed data |
| NEWS.md | Documented new feature |
| NAMESPACE | Exported new function |
Comments suppressed due to low confidence (1)
R/visualisation_recipe_internal.R:362
- The logic at lines 325 and 335 prevents showing both raw data and collapsed data, or both residuals and collapsed data. When
collapse_byis specified, it will only show collapsed data (lines 349-362), but not the raw data or residuals. This means users cannot layer both raw and collapsed data in the same plot. While this might be intentional, consider whether this limitation aligns with expected user behavior or if it should be documented more clearly.
if (show_data && is.null(collapse_by)) {
layers[[paste0("l", l)]] <- .visualization_recipe_rawdata(x, aes, numeric_as_discrete)
# Update with additional args
if (!is.null(point)) {
layers[[paste0("l", l)]] <- utils::modifyList(layers[[paste0("l", l)]], point)
}
l <- l + 1
}
# add residual data as next lowest layer
if (show_residuals && is.null(collapse_by)) {
layers[[paste0("l", l)]] <- .visualization_recipe_residuals(
x,
aes,
numeric_as_discrete
)
# Update with additional args
if (!is.null(point)) {
layers[[paste0("l", l)]] <- utils::modifyList(layers[[paste0("l", l)]], point)
}
l <- l + 1
}
# next possible data layer is collapsing by group ---------------
if (!is.null(collapse_by) && (show_data || show_residuals)) {
layers[[paste0("l", l)]] <- .visualization_recipe_collapsed_data(
x,
aes,
collapse_by,
numeric_as_discrete,
show_residuals
)
# Update with additional args
if (!is.null(point)) {
layers[[paste0("l", l)]] <- utils::modifyList(layers[[paste0("l", l)]], point)
}
l <- l + 1
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
R/collapse_by_group.R
Outdated
There was a problem hiding this comment.
The function doesn't handle collapse_by = TRUE as documented. The documentation states that setting collapse_group = TRUE should collapse by the first random effect groups (see man/visualisation_recipe.estimate_predicted.Rd lines 84-86), but the implementation only checks is.null(collapse_by) at line 37. If a user passes collapse_by = TRUE, it will fail at line 49 with an error "Could not find TRUE column." To fix this, add a check like if (isTRUE(collapse_by)) before line 37 to handle this case.
Fixes #130