Update plot.means_by_group.R to keep order of categories

[elinw]

No description provided.

[gemini-code-assist]

Code Review

This pull request fixes a double assignment typo and converts the Category column to a factor in plot.means_by_group.R. The reviewer noted that using as.factor() will sort the categories alphabetically, potentially losing their original order. They suggested explicitly setting the factor levels using unique(trimmed$Category) to preserve the original order and drop unused levels.

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.

[strengejacke]

Please see the Gemini suggestion, as as.factor(trimmed$Category) indeed uses alphabetical order.

[elinw]

Please see the Gemini suggestion, as as.factor(trimmed$Category) indeed uses alphabetical order.

That was fixed when I made the comment.
https://github.com/elinw/see/blob/12ded91a7712959c40840bbd002f2296b8e75403/R/plot.means_by_group.R#L40

I'm not sure exactly what is going on with the diff and the comments but that change was made previously (which is why I only explained why I didn't add the unique().

[strengejacke]

No, there's also a second as.factor(), where you need to set levels (including unique() to avoid duplicated levels)

[elinw]

No, there's also a second as.factor(), where you need to set levels (including unique() to avoid duplicated levels)

It's literally a one line proposed change (2 if you count fixing the duplication in the prior line). There is not a second as.factor() and actually as.factor() strangely does not have levels as an argument so you can't even do that! Therefore my one line suggestion uses factor(). So I'm not sure even what line you are referring to. Can you please share a link to this "second as.factor()"? Actually, I think you may be thinking of the first frustrated patch-1 which I changed to factor subsequently. I searched the whole file for as.factor and there is not one.

The factor() function itself will error if given a vector of levels that contains duplicates

factor(levels = c("A", "A", "B"))

So that's another reason why checking for duplicates is redundant.

Also because plotmeans_by_group() applies to an object created by datawizard::means_by_group() which already groups by the Category variable it cannot have duplicate rows unless you think someone will be modifying it to create duplicate rows. I'm trying to see why someone would do that and why, if they do that intentionally for some reason, the plot function should override that intent. But if you really want to take the small hit of unnecessary code, I'd be willing to add. I've already fixed this in my version of the function, but I do feel that not maintaining the order is a bug.
I think you could say that there should be some kind of special error if there are duplicates. That should really be done as validation at the beginning of the function I guess? Make sure it really is an object of the correct type not just using class name?

[elinw]

I think I found what you were talking about.

[strengejacke]

Thanks! Vac you also please add a unique() to the levels argument to avoid duplicated levels?

[elinw]

Thanks! Vac you also please add a unique() to the levels argument to avoid duplicated levels?

If you really want to take the performance hit despite what I explained about how it is impossible for there to be duplicates. But what will happen if there are duplicates? I haven't tested that but what's ggplot going to do with that? Maybe if there are duplicates the names shoul be changed.?

so e.g. check for duplicates

if (length(x$Categories) != length(unique(x$Categories)) {
   error("Category names are not unique.")
}

[strengejacke]

If you have more than one variable in select, you end up having duplicates in the plot method for $Categories.

[elinw]

If you have more than one variable in select, you end up having duplicates in the plot method for $Categories.

Done and added basic tests.

[strengejacke]

Now you committed the ‎R/plot.dw_groupmeans.R‎ file again, which contains the old code. Could you please remove it again?

I don't think we need the check for the length if we use unique(), so you can safely remove this code:

if (length(trimmed$Category) == length(unique(trimmed$Category))) {
    trimmed$Category <- factor(trimmed$Category, levels = trimmed$Category)
  } else {
    stop("Duplicate category names are present.")
  }

[strengejacke]

The commit just needs

  trimmed$Category <- factor(
    trimmed$Category,
    levels = unique(trimmed$Category)
  )