uclahs-cds / package-BoutrosLab-plotting-general

Functions to Create Publication-Quality Plots
https://uclahs-cds.github.io/package-BoutrosLab-plotting-general
11 stars 4 forks source link

Issue creating multipanel plot when dev is NULL #86

Open stefaneng opened 1 year ago

stefaneng commented 1 year ago

Running this code without a default plotting device results in an error (such as the cluster)

library(BoutrosLab.plotting.general);

x <- create.histogram(x = rnorm(10));
create.multipanelplot(list(x), filename = 'test.png');

Error in dev.off() : cannot shut down device 1 (the null device)

This is due to calling dev.off() when there is a filename but fails due to NULL dev: https://github.com/uclahs-cds/public-R-BoutrosLab-plotting-general/blob/main/R/create.multipanelplot.R#L936-L938

I suggest:

The check for null device is done correctly in the print function: https://github.com/uclahs-cds/public-R-BoutrosLab-plotting-general/blob/main/R/create.multipanelplot.R#L8

Good way of cleanly restoring the previous device here: https://github.com/tidyverse/ggplot2/blob/HEAD/R/save.r#L103-L108

aholmes commented 1 year ago

Good find! @RoniHaas just ran into this today as well. The error is caused when the drawing calls fail before any devices have opened. For example, if calling png(type='cairo') and libcairo cannot be loaded, dev is empty.

The second fix mentioned (checking dev.list()) is likely the best approach. It supports all cases when calling dev.off() is incorrect.

For a reference, here is another way we check dev.list(). https://github.com/uclahs-cds/public-R-BoutrosLab-plotting-general/blob/ac4d8516772a558b21b2a52fbe92aa72659298ee/R/create.heatmap.R#L810


In addition or as an alternative, we might want to consider falling back to any of the other default type values used by the png, tiff, etc. functions if the error is caused by, e.g., libcairo missing. This will allow us to support a wider variation of machine configurations.


Regarding fixing the error on machines in the meantime, it seems that installing libcairo will mask the problem and allow the calls to succeed. On Roni's machine we installed brew, then used it to install libcairo. We also installed xquartz, and the problem was resolved. If we see this on another machine, let's try installing just xquartz to see if that fixes it.

jsahrmann commented 1 year ago

I have an old Mac laptop that I used to investigate further over the weekend. I did a factory reset, then installed R, RStudio, and PBG, and I was able to get the same error Roni was seeing on Friday. Installing XQuartz was enough to fix the problem; I didn't need to install homebrew/libcairo. (Note that the R for macOS page actually references XQuartz as specifically being required under the 'Latest release:' heading.)

I think the simplest solution to avoid the error is to change the offending lines per @aholmes suggestion. The dev.off calls associated with the error happen in the helper functions get.legend.width, get.legend.height, get.text.grob.width, and get.text.grob.height. However, users without cairo could still run into errors. Each of the four helper functions defines a variable bitmap.type as the value of getOption('bitmapType'). They then check if cairo is available using capabilities('cairo'), and if it is available, set bitmap.type to 'cairo'. bitmap.type is passed to the tiff and png functions through the type parameter. Presumably if cairo isn't available, the functions are able to use whatever getOption('bitmapType') returned originally without issue. But users passing a file name with a .pdf extension would still get an error because the helper functions call cairo_pdf rather than the ordinary pdf function. Adding a fallback for PDF output could help here.

aholmes commented 1 year ago

Thanks, @jsahrmann! Good finds. As you mentioned face-to-face, Stefan may have run into this on the cluster. I remembered that I happen to be working on the libcairo dependencies on the cluster, which is likely directly related to this. Good to know the fix for macs - as you suggested, let's talk with Stefan when he is available.

aholmes commented 1 year ago

Presumably if cairo isn't available, the functions are able to use whatever getOption('bitmapType') returned originally without issue.

This is a good idea, however, we may need to run this past Paul. BPG exists to help make very specific kinds of charts, and allowing some defaults like this may not be the intent of the library.

stefaneng commented 1 year ago

@aholmes @jsahrmann Yes this was done when I was doing the non-conda R install so missing libcairo. We could add a warning about missing cairo on package load. I think that regardless of whether cairo is installed we should still add checks for dev.list() being null as it is possible a user changed the device manually or another package modified the device.

jsahrmann commented 1 year ago

Here's another complication around cairo: I reset my Mac again to remove XQuartz and then manually ran the checks used by, e.g., get.legend.width. The results are perplexing:

Screenshot 2023-01-30 at 6 58 59 PM

So capabilities('cairo') returns TRUE, but if I try to use cairo to make a plot, R crashes because it's not able to load cairo. I'm not sure if this is a Mac issue or something more general. capabilities seems like the right way to test for cairo, but it clearly gives misleading results in this case. This complicates the task of detecting when the fallback option would be necessary. I suppose we could resort to tryCatch. But per @aholmes's comment, if the point of PBG is to facilitate making graphs in a particular way, then perhaps offering a fallback option that would deviate from the ideal isn't worth the trouble.