ropensci / daiquiri

Data quality reporting for temporal datasets.
https://ropensci.github.io/daiquiri/
GNU General Public License v3.0
35 stars 2 forks source link

Issue with upcoming ggplot2 #19

Closed teunbrand closed 9 months ago

teunbrand commented 10 months ago

Hello there,

We have been preparing a new release of ggplot2 and during a reverse dependency check, it became apparent that the prospective ggplot2 3.5.0 would break daiquiri.

Unfortunately we weren't able to diagnose what is causing the error, but our check appears to point to the plot_overview_combo_static() function. It is entirely posible that this might be a false positive failure, but we'd like to report on it anyway to give an early heads up.

To test the code changes with the release candidate, you can install it with the code below:

remotes::install_github("tidyverse/ggplot2", ref = remotes::github_pull("5592"))

The release of ggplot2 3.5.0 is scheduled for the 12th of Februari. The progress of the release can be tracked in https://github.com/tidyverse/ggplot2/issues/5588.

phuongquan commented 10 months ago

Thanks for the heads up, I've tracked down the error.

It happens when I use geom_tile() and all my data values are NA, and in scale_fill_gradient() I set one of the limits to NA (i.e. ask it to use the existing minimum or maximum). e.g.

testdata_empty <-
  data.frame(
    datefield = seq(as.Date("2000/1/1"), by = "month", length.out = 12),
    heatmap_row = factor(c(rep("row 1", 12), rep("row 2", 12))),
    heatmap_value = rep(NA_integer_, 24)
  )

ggplot2::ggplot(testdata_empty,
                ggplot2::aes(datefield, heatmap_row, fill = heatmap_value)) +
  ggplot2::geom_tile() +
  ggplot2::scale_fill_gradient(
    low = "white",
    high = "red",
    na.value = "grey",
    labels = NULL,
    limits = c(0, NA)
  )

which returns the error

Error in seq.default(limits[1], limits[2], length.out = nbin) : 
  'to' must be a finite number

Interestingly, I don't get the error if I set both limits to NA, in which case the plot uses the na.value colour for all tiles as desired.

Is this something that will be fixed before the next release or do I need to update daiquiri to work around it? (Note, I do need to be able to create plots that contain all NAs.)

Thanks

teunbrand commented 10 months ago

Thanks for tracking down the bug! This sounds like something ggplot2 should fix instead of daquiri.

teunbrand commented 9 months ago

Considered fixed by https://github.com/tidyverse/ggplot2/pull/5681