716 plot methods do not always work as expected

[Puzzled-Face]

Fixes #716 .

[github-actions]

Code Coverage Summary

Filename                    Stmts    Miss  Cover    Missing
------------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/checkmate.R                  73       1  98.63%   72
R/crmPack-package.R             4       0  100.00%
R/Data-class.R                147       5  96.60%   43, 564-565, 571-576
R/Data-methods.R              203       0  100.00%
R/Data-validity.R             144       1  99.31%   21
R/Design-class.R              319       0  100.00%
R/Design-methods.R           2763    1874  32.18%   111, 136-139, 147-158, 162-181, 193-197, 203-216, 392-516, 659-663, 687-690, 698-717, 722-751, 755-756, 758, 773-781, 785, 797-816, 1034-1375, 1426, 1616, 1787-3971, 4069, 4071-4072, 4131, 4167, 4204-4207, 4226-4230, 4291-4298, 4326-4330, 4338-4356, 4382-4401, 4404, 4437, 4459-4477, 4711
R/Design-validity.R            20       0  100.00%
R/fromQuantiles.R             178      69  61.24%   237-378
R/helpers_covr.R               23       0  100.00%
R/helpers_data.R               84       1  98.81%   139
R/helpers_design.R             75      35  53.33%   77-129
R/helpers_jags.R               77       0  100.00%
R/helpers_model.R              78       4  94.87%   38, 89-90, 139
R/helpers_rules.R             428       0  100.00%
R/helpers_simulations.R        18       0  100.00%
R/helpers.R                   229      65  71.62%   107-127, 140, 153-167, 203-323, 358-372
R/logger.R                     11       0  100.00%
R/mcmc.R                      287      18  93.73%   90-95, 374-375, 385, 387-388, 391-394, 577-578, 667, 673, 731
R/McmcOptions-class.R          22       0  100.00%
R/McmcOptions-methods.R         8       1  87.50%   43
R/McmcOptions-validity.R       42       0  100.00%
R/Model-class.R              1012     155  84.68%   145-147, 216-218, 222-224, 283-285, 357-359, 363-365, 444-446, 513-515, 577-581, 584-587, 690-693, 697-698, 813-817, 937-939, 943-951, 1096-1098, 1103-1106, 1110-1113, 1229-1233, 1235-1238, 1242-1245, 1248, 1409-1419, 1424-1430, 1585-1588, 1594-1601, 1758, 1767, 1776, 1785, 1794-1799, 1935, 1944, 1953, 1961-1963, 2807-2836, 2840-2846, 2853-2857, 2862, 2969-2982, 3008, 3104-3106, 3110, 3203-3205, 3209, 3278-3290, 3308
R/Model-methods.R             376      34  90.96%   78, 155, 208, 675-720, 1039-1048
R/Model-validity.R            435       8  98.16%   430-433, 442-445
R/ModelParams-class.R          17       0  100.00%
R/ModelParams-validity.R       21       0  100.00%
R/Rules-class.R               431       2  99.54%   3150, 3231
R/Rules-methods.R            1429     164  88.52%   741, 744, 887, 890, 893, 1008, 1011, 1014, 1134-1137, 1171, 1274-1277, 1312, 2475-2483, 2507-2514, 3017-3260
R/Rules-validity.R            418       0  100.00%
R/Samples-class.R               6       0  100.00%
R/Samples-methods.R          1104      45  95.92%   406-416, 644, 1358-1393, 1616, 1629, 1794, 2124-2129
R/Samples-validity.R           10       0  100.00%
R/Simulations-class.R         190      28  85.26%   601-613, 708-718, 770-773
R/Simulations-methods.R      1632    1488  8.82%    65-354, 410, 420-439, 452-457, 504-513, 679-2972
R/Simulations-validity.R       75      75  0.00%    20-168
R/utils.R                       6       0  100.00%
TOTAL                       12395    4073  67.14%

Diff against main

Filename                   Stmts    Miss  Cover
-----------------------  -------  ------  -------
R/Simulations-methods.R      +28     +28  -0.15%
TOTAL                        +28     +28  -0.15%

Results for commit: 194609f

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[danielinteractive]

Thanks @Puzzled-Face for working on this, my concern with the grid::grid.draw() wrapper applied everywhere is that 1) this will no longer return the plot objects (because the value of it is NULL) and 2) it will always plot things on the device, where the user might just want to save plots first and look at them later.

[danielinteractive]

I think we also don't have more in the title for the other data classes, so don't need that here then either?

[Puzzled-Face]

Thanks @Puzzled-Face for working on this, my concern with the grid::grid.draw() wrapper applied everywhere is that 1) this will no longer return the plot objects (because the value of it is NULL) and 2) it will always plot things on the device, where the user might just want to save plots first and look at them later.

Hmm. Yes. Good point. (Though grid.draw is used in several places in the package already...) An alternative then would be to replace with calls to (eg) gridExtra::grid.arrange(), ggpubr::ggarrange() or cowplot::plot_grid(). gridExtra::grid.arrange() appears to be the best option as the other two would introduce a new dependency. Or, less appealing (IMHO), would be to simply return a list and expect the end-user to sort things out for themselves - which would be a breaking change.

Should I also remove all other calls to grid.draw, or are they appropriate elsewhere? (I am suspicious of graphs which always appear on an interactive device...)

[danielinteractive]

@Puzzled-Face the problem is that also grid.arrange directly plots the object. My wish with the print and plot methods was to decouple the actual computation of the plot from the plotting (as it works e.g. with standard ggplot2 objects)

[Puzzled-Face]

@Puzzled-Face the problem is that also grid.arrange directly plots the object. My wish with the print and plot methods was to decouple the actual computation of the plot from the plotting (as it works e.g. with standard ggplot2 objects)

I should read the online doc more carefully!

We can wrap ggpubr::grid.arrange() in ggpubr::as_ggplot(). The documentation for cowplot::plot_grid() is incomplete, but the function does appear to return a plot object:

library(cowplot)
library(tidyverse)

set.seed(123)
d <- lapply(1:3, \(i) tibble(x = rnorm(5), y = rnorm(5)))
p <- lapply(d, \(df) df %>% ggplot() + geom_point(aes(x =x, y = y)))
g <- plot_grid(plotlist = p)
class(g)
[1] "gg"     "ggplot"

ggpubr::ggarrange() returns a ggplot or a list of ggplots.

library(ggpubr)
library(tidyverse)

set.seed(123)
d <- lapply(1:3, \(i) tibble(x = rnorm(5), y = rnorm(5)))
p <- lapply(d, \(df) df %>% ggplot() + geom_point(aes(x =x, y = y)))

g <- ggarrange(plotlist = p)
class(g)
[1] "gg"        "ggplot"    "ggarrange"

Reverting #697 remains an option.

How would you like to proceed?

[danielinteractive]

Thanks @Puzzled-Face , to me reverting #697 still seems easiest but also most clean from a design perspective (basically we just want that the gtable object behaves as a ggplot2 object and plots when we print it)