-
-
Notifications
You must be signed in to change notification settings - Fork 15
Remove ggmosaic from v0.5.0 #935
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
R/tm_g_bivariate.R
Outdated
| env = list(xval = x, yval = y) | ||
| ) | ||
| ) | ||
| stop("Classes for 'x' and 'y' are currently not supported.") |
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.
Is this the quick fix that was discussed with the team?
Maybe we can find a way to create our own version of ggmosaic::geom_mosaic or find a substitute in a different package.
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.
Yes, we decided to remove ggmosaic and not implement our own version. @gogonzo made some research and didn't found any other package we could substitute it with (on the PR I propose plot(table()) if needed.
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.
ok
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.
@llrs-roche
Contingency table is a good proposition. Also, I've just found graphics::mosaicplot - please explore if it makes sense.
Remaining problem for Contingency table and graphics::mosaicplot is facetting - from data perspective facetting is just extra grouping dimension.
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.
plot.table uses mosaicplot() (except for the 1D table):
Differences between plot.table and mosaicplot on 1D tables
par(mfrow = c(1, 2))
plot(table(CO2$Plant))
mosaicplot(CO2$Plant)
mosaicplot() deals with more dimensions by grouping them, see for example: plot(table(CO2), color = TRUE):
mosaicplot() help page points to vsd::mosaic(), which besides some extra color output it provides some statistical output if needed.
However, I don't think it is worth the complexity it would add:
- Maybe I'm not used to this kind of tables, but reading the above table is tricky, because to me it is not clear what it means (Mississippi type, Mc2 plant, Treatment chilled where is it?). Maybe users of this module are more familiar with it
- currently all the function stack assumes we are generating a ggplot2 call. Even the module has some specific ggplot2 options that would apply.
- Producing plots with
plot.tableorvsd::mosaic()in some cases and some other cases with ggplot2 would also make it harder to write decorators. - Replacing ggmosaic with vsd keeps the number of dependencies as high as currently is, which makes us vulnerable to breaking changes/updates on any of those packages. Using R default
plot.table/mosaicplothas all the previous problems. - I searched for internal repositories using this module and the ones I found using it aren't updated in a while (~5 years). So I don't think internal users would be affected much by removing support to this.
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.
are you guys still exploring alternatives in here?
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.
No, I don't think it is worth the effort to add a non ggplot2 alternative
|
Checks are green (I already fixed the spelling issue on e9bcdb7: https://github.com/insightsengineering/teal.modules.general/actions/runs/18675789589/job/53245400137 |
Pull Request
Fixes #932
Removes ggmosaic dependency and updates code as required by the new error message.
Tested with released packages and also checked the app generated on the vignette (using-bivariate-plot) still works well on all shown cases.
The user impact might be higher than initially thought as logical, character an ordered classes also use ggmosaic due to L848.
As an alternative we could use
plot(table())which is not ggplot2 based but works with factors and characters.Reproducible example