Preserve order of categories in the plot#448
Conversation
Remove unneeded addition
There was a problem hiding this comment.
Code Review
This pull request introduces new S3 plot methods for grouped means (plot.dw_groupmeans and plot.dw_groupmeans_list) and crosstabs (plot.datawizard_crosstab), along with corresponding documentation and test suites. The code review highlights several critical and high-severity issues, including an undefined variable t_base in the crosstab plot function, a syntax error in the documentation examples, and compatibility issues with the native pipe operator |> on older R versions. Additionally, the feedback suggests improvements to confidence interval limit calculations (handling NA and negative values), ensuring category ordering is preserved in list plots, avoiding explicit print() calls in S3 plot methods, and correcting documentation tags.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| x_long <- datawizard::data_to_long( | ||
| x, | ||
| rows_to = "row_var", | ||
| select = names(t_base)[-1] |
There was a problem hiding this comment.
This all was included by accident see new pR
| #' group_means_object <- datawizard::means_by_group( | ||
| #' iris$Sepal.Width, iris$Species) | ||
| #' ) |
There was a problem hiding this comment.
| if ("CI_low" %in% names(trimmed)) { | ||
| lower_lim <- .9 * min(trimmed$CI_low) | ||
| upper_lim <- 1.1 * max(trimmed$CI_high) | ||
| } |
There was a problem hiding this comment.
The current calculation of lower_lim and upper_lim has several issues:\n1. It does not handle NA values, which will cause min() and max() to return NA unless na.rm = TRUE is specified.\n2. It breaks for negative values (e.g., multiplying a negative minimum by 0.9 makes it larger/closer to zero, which truncates the lower part of the confidence interval).\n\nA more robust approach is to calculate the range of the confidence intervals and expand the limits relative to that range.
if ("CI_low" %in% names(trimmed)) {\n ci_range <- max(trimmed$CI_high, na.rm = TRUE) - min(trimmed$CI_low, na.rm = TRUE)\n lower_lim <- min(trimmed$CI_low, na.rm = TRUE) - 0.1 * ci_range\n upper_lim <- max(trimmed$CI_high, na.rm = TRUE) + 0.1 * ci_range\n }There was a problem hiding this comment.
This is wrong since these are never NA.
|
|
||
| x_long <- do.call(rbind, x) | ||
|
|
||
| trimmed <- datawizard::data_filter(x_long, Category != "Total") |
There was a problem hiding this comment.
In plot.dw_groupmeans_list, trimmed$Category is not converted to a factor. This means the main objective of this PR (preserving the order of categories in the plot) will not be applied when plotting a list of group means. We should convert trimmed$Category to a factor here as well, using unique(trimmed$Category) to handle the duplicate categories across different variables.
trimmed <- datawizard::data_filter(x_long, Category != "Total")\n trimmed$Category <- factor(trimmed$Category, levels = unique(trimmed$Category))| if ("CI_low" %in% names(trimmed)) { | ||
| lower_lim <- .9 * min(trimmed$CI_low) | ||
| upper_lim <- 1.1 * max(trimmed$CI_high) | ||
| } |
There was a problem hiding this comment.
Just like in plot.dw_groupmeans, the calculation of lower_lim and upper_lim here does not handle NA values and breaks for negative values. Let's use the same robust range-based expansion.
if ("CI_low" %in% names(trimmed)) {\n ci_range <- max(trimmed$CI_high, na.rm = TRUE) - min(trimmed$CI_low, na.rm = TRUE)\n lower_lim <- min(trimmed$CI_low, na.rm = TRUE) - 0.1 * ci_range\n upper_lim <- max(trimmed$CI_high, na.rm = TRUE) + 0.1 * ci_range\n }| #' (if present in the object should be graphed), and `caption` for if | ||
| #' a caption summarizing ANOVA results should be included. | ||
| #' | ||
| #' @value |
| caption_text = "" | ||
| } | ||
| trimmed <- datawizard::data_filter(x, Category != "Total") | ||
| trimmed$Category <- factor(trimmed$Category, levels = trimmed$Category) |
There was a problem hiding this comment.
When converting Category to a factor, it is safer to use unique(trimmed$Category) for the levels argument. While duplicates might not be expected in a single group means table, using unique() is a more robust and defensive programming practice.
trimmed$Category <- factor(trimmed$Category, levels = unique(trimmed$Category))There was a problem hiding this comment.
These can never be duplicates because of how the object is created.
|
|
||
| x_attributes <- lapply(x, attributes) | ||
| x_var_names <- lapply(x_attributes, `[[`, "var_mean_label") | ||
| x_captions <- mapply(.build_caption, x, x_var_names) |
There was a problem hiding this comment.
Using mapply with list arguments that can contain NULL elements (such as x_var_names if the "var_mean_label" attribute is missing) can lead to unexpected recycling behavior or errors in R. Using vapply with an index is much safer and guarantees a character vector output.
x_captions <- vapply(seq_along(x), function(i) {\n .build_caption(x[[i]], x_var_names[[i]])\n }, character(1))| p <- p + plotlist | ||
| print(p) |
There was a problem hiding this comment.
There was a problem hiding this comment.
This is incorrect when the plot is produced inside a loop.
| if (is.null(proportion_type)) { | ||
| x_long | ||
| plotlist[[1]] <- aes(x = row_var, y = name, fill = value) |
|
There are a variety of places where this code won't work as expected. Could you give a reproducible example of a case where the levels of a factor aren't being preserved as expected with the current version of the package? Note--if you haven't converted the variable to factor before modeling, that is the best place to do so |
|
We already have methods for plotting objects returned by The relevant code is here: The current implementation is also more elegant as it handles arguments directly, which makes the code in parts shorter and easier to maintain. We don't need to capture all the arguments via |
|
Sorry sent the wrong branch |
|
Hi
Lol that was contributed by me, though in fact |
I noticed that when the category names are not in alphabetical order, ggplot2 alphabetizes when defining the x axis. This matters because sometimes order is important (in the case I noticed they defined a Likert scale). From what I learned doing some reading the way to address this is to turn the Category column into a factor. THat's what this PR does. That may have other consequences, I'm not sure yet.