Skip to content

Keep category order in group means plot#450

Closed
elinw wants to merge 1 commit into
easystats:mainfrom
elinw:keep_category_order
Closed

Keep category order in group means plot#450
elinw wants to merge 1 commit into
easystats:mainfrom
elinw:keep_category_order

Conversation

@elinw

@elinw elinw commented Jul 7, 2026

Copy link
Copy Markdown

See #448

@gemini-code-assist

Copy link
Copy Markdown

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@strengejacke

Copy link
Copy Markdown
Member

Thanks! I have basically the same comments as in #448

We already have methods for plotting objects returned by means_by_group(), you should build upon that code instead of re-implementing a second plot() method, especially, since plot() for that class (.dw_groupmeans) is already defined in the datawizard package.

The relevant code is here:
https://github.com/easystats/see/blob/main/R/plot.means_by_group.R

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 ....

If you would like to commit any changes to the plot() method for means_by_group(), please build upon the current implementation: https://github.com/easystats/see/blob/main/R/plot.means_by_group.R

in fact ... methods are easier to maintain because you don't have to add everything users suggest

This makes it harder for users to understand which arguments are supported, because they're not explicitly documented.

they don't violate the OOP principle of matching the signature of the parent method.

The signatures we have in the other packages work fine. See https://github.com/easystats/datawizard/blob/main/R/means_by_group.R. That's why we don't need to handle dot-arguments, but can make additional arguments explicit. This code is then much easier to read and to maintain.

@elinw

elinw commented Jul 8, 2026

Copy link
Copy Markdown
Author

I'm just adding one line to your code to make the category be treated as a factor.
Also why is the line like this, with trimmed <- twice ?

 trimmed <- trimmed <- x_long[x_long$Category != "Total", ]

If you look at my pr for that line (which was the basis for the creation of the method) it is not like that and I don't really see what it adds to do it like that.

@elinw elinw closed this Jul 8, 2026
@elinw

elinw commented Jul 8, 2026

Copy link
Copy Markdown
Author

Overall I think you are misreading this PR which, yes, does look odd but literally does just add one line.

@strengejacke

Copy link
Copy Markdown
Member

No, it adds 192 lines:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants