Skip to content

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 21, 2025
Merged

Remove ggmosaic from v0.5.0 #935

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
78da68f
Prepare for removing ggmosaic
llrs-roche Oct 20, 2025
fc313d6
Remove ggmosaic
llrs-roche Oct 20, 2025
e95e19c
Remove ggmosaic fron imports
llrs-roche Oct 20, 2025
a21fac7
Check error messages
llrs-roche Oct 20, 2025
1a74b44
Remove any reference to ggmosaic
llrs-roche Oct 20, 2025
601d3a9
Add NEWS piece
llrs-roche Oct 20, 2025
b045167
Use one variable for all the error messages
llrs-roche Oct 20, 2025
6eba244
Remove programming terms from error message
llrs-roche Oct 20, 2025
ff2b882
Word improvement
llrs-roche Oct 20, 2025
54ac71a
Remove ggmosaic from pre-commit
llrs-roche Oct 20, 2025
e9bcdb7
Fix spelling "issue"
llrs-roche Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ URL: https://insightsengineering.github.io/teal.modules.general/,
BugReports:
https://github.com/insightsengineering/teal.modules.general/issues
Depends:
ggmosaic (>= 0.3.0),
ggplot2 (>= 3.4.0),
R (>= 4.1),
shiny (>= 1.8.1),
Expand Down
1 change: 0 additions & 1 deletion NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ export(tm_missing_data)
export(tm_outliers)
export(tm_t_crosstable)
export(tm_variable_browser)
import(ggmosaic)
import(ggplot2)
import(shiny)
import(teal)
Expand Down
7 changes: 0 additions & 7 deletions R/teal.modules.general.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#' (not necessarily for clinical trials data).
#'
#' @import ggplot2
#' @import ggmosaic
#' @import shiny
#' @import teal
#' @import teal.transform
Expand All @@ -15,11 +14,5 @@
#' @keywords internal
"_PACKAGE"

# nolint start
# Note ggmosaic (version <= 0.3.3) needs to be in DEPENDS as the following does not work if it is imported
# df <- data.frame(x = c("A", "B", "C", "A"), y = c("Z", "Z", "W", "W"))
# ggplot(df) + ggmosaic::geom_mosaic(aes(x = ggmosaic::product(x), fill = y))
# nolint end

# Needed to avoid R CMD note on no visible binding
utils::globalVariables("count")
2 changes: 1 addition & 1 deletion R/tm_g_association.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ srv_tm_g_association <- function(id,
)

qenv <- reactive(
teal.code::eval_code(data(), 'library("ggplot2");library("dplyr");library("tern");library("ggmosaic")') # nolint quotes
teal.code::eval_code(data(), 'library("ggplot2");library("dplyr");library("tern")') # nolint quotes
)
anl_merged_q <- reactive({
req(anl_merged_input())
Expand Down
8 changes: 1 addition & 7 deletions R/tm_g_bivariate.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,13 +968,7 @@ bivariate_ggplot_call <- function(x_class,
)
# Factor and character plots
} else if (x_class == "factor" && y_class == "factor") {
plot_call <- reduce_plot_call(
plot_call,
substitute(
ggmosaic::geom_mosaic(aes(x = ggmosaic::product(xval), fill = yval), na.rm = TRUE),
env = list(xval = x, yval = y)
)
)
stop("Classes for 'x' and 'y' are currently not supported.")
Copy link
Contributor

@m7pr m7pr Oct 20, 2025

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.

Copy link
Contributor Author

@llrs-roche llrs-roche Oct 20, 2025

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.

Copy link
Contributor

@m7pr m7pr Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor

@gogonzo gogonzo Oct 20, 2025

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.

Copy link
Contributor Author

@llrs-roche llrs-roche Oct 20, 2025

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)
image

mosaicplot() deals with more dimensions by grouping them, see for example: plot(table(CO2), color = TRUE):

image

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.table or vsd::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/mosaicplot has 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.

Copy link
Contributor

@m7pr m7pr Oct 21, 2025

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?

Copy link
Contributor Author

@llrs-roche llrs-roche Oct 21, 2025

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

} else {
stop("x y type combination not allowed")
}
Expand Down
28 changes: 14 additions & 14 deletions tests/testthat/test_bivariate_ggplot_call.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,33 +19,33 @@ testthat::test_that("bivariate_ggplot_call with numerics", {
})

testthat::test_that("bivariate_ggplot_call with factor, char, logical", {
testthat::expect_match(
testthat::expect_error(
bivariate_ggplot_call("factor", "factor") %>% deparse(width.cutoff = 300),
"geom_mosaic"
"Classes for 'x' and 'y' are currently not supported."
)
testthat::expect_match(
testthat::expect_error(
bivariate_ggplot_call("logical", "factor") %>% deparse(width.cutoff = 300),
"geom_mosaic"
"Classes for 'x' and 'y' are currently not supported."
)
testthat::expect_match(
testthat::expect_error(
bivariate_ggplot_call("character", "factor") %>% deparse(width.cutoff = 300),
"geom_mosaic"
"Classes for 'x' and 'y' are currently not supported."
)
testthat::expect_match(
testthat::expect_error(
bivariate_ggplot_call("logical", "character") %>% deparse(width.cutoff = 300),
"geom_mosaic"
"Classes for 'x' and 'y' are currently not supported."
)
testthat::expect_match(
testthat::expect_error(
bivariate_ggplot_call("character", "logical") %>% deparse(width.cutoff = 300),
"geom_mosaic"
"Classes for 'x' and 'y' are currently not supported."
)
testthat::expect_match(
testthat::expect_error(
bivariate_ggplot_call("logical", "logical") %>% deparse(width.cutoff = 300),
"geom_mosaic"
"Classes for 'x' and 'y' are currently not supported."
)
testthat::expect_match(
testthat::expect_error(
bivariate_ggplot_call("character", "character") %>% deparse(width.cutoff = 300),
"geom\\_mosaic"
"Classes for 'x' and 'y' are currently not supported."
)
})

Expand Down