search

thomasp85 / grid

personal devel version of grid
15 stars 4 forks source link

Issues with new unit implementation #14

Closed thomasp85 closed 4 years ago

thomasp85 commented 5 years ago

The problem is the resize_heights() function, which plays silly games to try to perform a unit subset assignment. I think that this might be fixed by just relying on grid's unit subset assignment, but I need to test on R with the new-unit patch applied.

Packages Gmisc ggfittext ggrepel ggstatplot: check class() assuming length 1, a bug in those packages. We reported Gmisc: I will leave the others until this is live.

GGally and codebook give some context:

Error in grid.Call.graphics(C_setviewport, vp, TRUE): VECTOR_ELT() can only be applied to a 'list', not a 'double'

as before.

from ggstatsplot

thomasp85 commented 5 years ago

cc @pmur002

thomasp85 commented 5 years ago

@pmur002 will it be possible to do a CRAN check soonish again with the #1 PR? I've done a fair amount of work to accommodate old units in the new code so that they should be converted transparently, and it would be nice to see how many issues this fix before tackling the remaining part...

pmur002 commented 5 years ago

I am not keen to take this anywhere near the official CRAN until we have things much more under control. However, I am developing my own testing set up that I can try testing on. It is based on a Docker image that ...

I can also run this on a 20-core 64GB machine, so it does not take all day (just a few hours).

I have got this producing something similar to the CRAN failures in the list above, so we can hopefully use it to bring the list of problems down much lower.

Will try to set a test run going with the latest new-unit version.

thomasp85 commented 5 years ago

That sounds good... let me try to fix the problems you reported before you begin testing

pmur002 commented 5 years ago

Ok. Let me know when you're ready for me to try a test.

thomasp85 commented 5 years ago

I've fixed the problems you identified, so you are free to try out a revdep check

pmur002 commented 5 years ago

Thanks. My upgradeUnit() tests now work for me (though I had to export the upgradeUnit() methods in NAMESPACE first - I have pushed that change).

pmur002 commented 5 years ago

I am updating my test set up (for latest CRAN package versions). Will run devdep check next week.

pmur002 commented 5 years ago

Sorry about the delay - teaching prep took over. I have run a set of checks and unfortunately, it looks like we are not there yet ...

Check status summary:
                  ERROR WARN NOTE  OK
  Source packages   294   29   66 106

May even be worse than before ?

That is just on direct 'grid' dependencies. Looking at the first error, it seems like we break 'lattice' straight off the bat. For example, this fails ...

library(lattice)
example(barchart)

Do you have time to take a look at that?

the check summary is attached, which is useful for seeing which packages failed. I can put the full set of check directories up on a dropbox for you if you like, but it is 2GB. Alternatively, I can provide individual package check directories if you want to take a closer look at anything.

workResult.txt

pmur002 commented 5 years ago

I have tracked the first problem (in 'abd') to a much simpler issue in 'lattice'. I would say the problem probably also afflicts 'ggplot2' (unconfirmed), which would explain the blow out in package errors (?). Anyway, the problem is as simple as ...

max(unit(1, "strwidth", "test"))

... which produces ...

Error in upgradeUnit.default(list(list(1, "test", 14L))) : 
  Not a unit object

This is traced to the following lines in Summary.unit() ...

  x <- unlist(lapply(units, as.unit), recursive = FALSE)

  matchUnits <- .Call(C_matchUnit, x, ok)

... where the unlist() downgrades the list of "unit" objects in 'x' to a list of plain lists. C_matchUnit() eventually chokes on that because a plain list is not a "unit" object. The rest of the code in Summary.unit() appears to be based on a list of "unit" objects, which suggests that the unlist() is wrong, but it does not look like C_matchUnit() is designed for a list of "unit" objects. I am not sure what the right fix is here - any suggestions? Is my diagnosis correct ?

thomasp85 commented 5 years ago

I'm back from vacation and will begin working on a fix for this — thanks for the initial digging

thomasp85 commented 5 years ago

If only ever all bugs were this easy to fix :-)

pmur002 commented 5 years ago

Got it. Thanks. Will run another grid rev dep check.

pmur002 commented 5 years ago

I am now seeing ...

> xyplot(Sepal.Length + Sepal.Width ~ Petal.Length + Petal.Width | Species,
  data = iris, scales = "free", layout = c(2, 2),
  auto.key = list(x = .6, y = .7, corner = c(0, 0)))

Error: index out of bounds ("unit" subsetting)

I'm beginning to suspect we should at least get 'lattice' and 'ggplot2' passing R CMD check before I run the larger/longer tests :)

thomasp85 commented 5 years ago

I have fixed the error above and both lattice and ggplot2 now passes tests with the new-unit branch. Can I get you to once more run a full batteri of revdep checks?

pmur002 commented 5 years ago

Thanks! Will try revdep over next day or two.

pmur002 commented 5 years ago

An interim report to show progress. We are down to "only" 42 package failures (see below). I am looking closer at these because there are some suspicious results (e.g., 'ggplot2' reports error, but without the massive cascade of errors to dependent packages).

AeRobiology ... ERROR analogue ... ERROR animint2 ... ERROR bcrm ... ERROR CARBayes ... ERROR Cascade ... ERROR ChainLadder ... ERROR ChemoSpec ... ERROR clustrd ... ERROR clustree ... ERROR cRegulome ... ERROR genoPlotR ... ERROR ggExtra ... ERROR ggplot2 ... ERROR ggpol ... ERROR ggraph ... ERROR ggrepel ... ERROR ggResidpanel ... ERROR ggstatsplot ... ERROR gprofiler2 ... ERROR gridSVG ... ERROR heatwaveR ... ERROR hexbin ... ERROR HistDAWass ... ERROR LDheatmap ... ERROR lemon ... ERROR metR ... ERROR morse ... ERROR multipanelfigure ... ERROR NMF ... ERROR partykit ... ERROR pheatmap ... ERROR Plasmidprofiler ... ERROR portfolio ... ERROR Rcan ... ERROR survey ... ERROR tmap ... ERROR trelliscopejs ... ERROR ufs ... ERROR vcd ... ERROR vivo ... ERROR wilson ... ERROR

pmur002 commented 5 years ago

'AeRobiology' traces back to plotly:::mm2pixels(), which is peeking at attr(u, "unit") (strike 1), expecting it to always be there for units (strike 2), and expecting it to be a character value (strike 3). plotly:::verifyUnit() also peeks at attr(u, "unit").

pmur002 commented 5 years ago

'ggplot2' passes when tested on its own (it had failed on a testthat test reporting non-identical results for some axis labels - not sure what that was, but does not seem to be related to 'grid' units)

pmur002 commented 5 years ago

'analogue' passes when tested on its own (it had failed on LaTeX errors when creating PDF version of manual). Although I noted that the package version I downloaded manually from CRAN was newer than the one used for the revdep check (same thing for 'ggplot2') so I need to check my scripts that update packages for the revdep check.

pmur002 commented 5 years ago

My script was checking older versions of packages as well as the most recent (so some of those failures were from out-of-date packages). Rechecking now ...

thomasp85 commented 5 years ago

Well, only 42 including some false positives is def an improvement 🙂

FWIW, the issue with plotly where they inspect the unit is why I added the unitType() method. I think it will be easier to get people to update their packages if we provide an easy remedy

pmur002 commented 5 years ago

Good point about your unitType() function. I agree about making it easier for people to fix their packages. It would be good to start keeping a record of suggested fixes for packages.

Having fixed my checking code, I we are down to 28 problem packages. However, note that 'AeRobiology' no longer appears (because its failure is in rebuilding vignettes which does not appear in the overall package check result), so we have an underestimate of the impact on CRAN.

animint2 ... ERROR
cowplot ... ERROR
genoPlotR ... ERROR
ggpol ... ERROR
ggraph ... ERROR
ggrepel ... ERROR
ggResidpanel ... ERROR
gprofiler2 ... ERROR
gridSVG ... ERROR
heatwaveR ... ERROR
hexbin ... ERROR
LDheatmap ... ERROR
lemon ... ERROR
metR ... ERROR
multipanelfigure ... ERROR
NMF ... ERROR
ParamHelpers ... ERROR
partykit ... ERROR
pheatmap ... ERROR
Plasmidprofiler ... ERROR
portfolio ... ERROR
survey ... ERROR
tmap ... ERROR
trelliscopejs ... ERROR
ufs ... ERROR
vcd ... ERROR
vivo ... ERROR
wilson ... ERROR

Will begin working through those.

pmur002 commented 5 years ago

'animint2' seems to be using its own margin() function, which is making "unit" objects of its own design. Fixable by getting 'animint2' to use ggplot2::margin() ? (I have raised an issue https://github.com/tdhock/animint2/issues/31 on the 'animint2' repo)

This is NOT just restricted to the margin() function; 'animint2' has copies of LOTS of 'ggplot2', so it will require a significant update (and communication with Toby Dylan Hocking suggests that this will not happen anytime soon).

thomasp85 commented 5 years ago

Yes... My guess is they copied the margin function from ggplot2 some time ago (I fixed it in ggplot2 some releases ago to make it ready for the new grid)

pmur002 commented 5 years ago

'cowplot' dies because this produces a floating point exception ...

grid::unit.pmax(grid::unit(1,"cm") + grid::unit(.5, "npc"), NULL)

... because within grid:::pSummary(), the NULL gets through to summaryUnits() [unit.c] because conformingUnits() [unit.c], which is called by grid:::identicalUnits(), bails out as soon as it hits a non-simple unit. So there needs to be better protection higher up against NULL values. Could you take a look at where that protection should go (while I continue sifting through the CRAN failures) ?

Should be fixed now (after TLP changes to grid new units)

https://github.com/thomasp85/grid/issues/14#issuecomment-549293873

pmur002 commented 5 years ago

'genoPlotR' is throwing an error because the new validData() [unit.c] enforces length of unit data to exactly match length of unit vector. The old grid::valid.data() enforced length of unit data to be at least as long as length of unit vector. Was there a strong design reason for enforcing exactly equal? I cannot remember a strong design decision for enforcing >=, but backward compatibility is now one argument.

Should be fixed now (after TLP changes to grid new units)

https://github.com/thomasp85/grid/issues/14#issuecomment-549293873

pmur002 commented 5 years ago

'ggpol' is a nasty-looking one. It is peeking deep into 'ggplot2' structures, expecting to find a numeric value and getting a "unit" instead, for example (from facet_share.R) ...

unit(c(axes$y$left[[1]]$children$axis$widths[[tick_idx]],
       1,
       axes$y$left[[1]]$children$axis$widths[[tick_idx]]),
     c("pt", "grobwidth", "pt"),
     list(NULL, axes$y$left[[1]]$children$axis$grobs[[lab_idx]], NULL)

This may require major surgery.

pmur002 commented 5 years ago

'ggraph' just got updated (but I suspect you knew that already) so will leave until the next round of revdep.

pmur002 commented 5 years ago

'ggrepel' was an easy one! The function ggrepel::to_unit() has ...

class(x) == "unit"

... and changing to is.unit(x) fixes it all up.

I have raised an issue on the 'ggrepel' repo https://github.com/slowkow/ggrepel/issues/141

pmur002 commented 5 years ago

'ggResidpanel' traces to the same plotly::mm2pixel() problem as 'AeRobiology'

pmur002 commented 4 years ago

'gprofiler2' traces to the same plotly::mm2pixel() problem as 'AeRobiology'

pmur002 commented 4 years ago

I have submitted fix for 'gridSVG' to CRAN. This is supposed to work for R-release, R-devel, AND R-new-unit (by making use of the grid::unitType() recently submitted to R-devel).

pmur002 commented 4 years ago

'heatwaveR' is another plotly::mm2pixel() problem

pmur002 commented 4 years ago

'hexbin' fails because it generates an S4 class from the S3 unit class. The following fix seems to clear things up (hexViewport.R) ...

setOldClass(c("unit", "unit_v2"))
setOldClass(c("simpleUnit", "unit", "unit_v2"))

I have generated a PR for 'hexbin' (https://github.com/edzer/hexbin/pull/12).

pmur002 commented 4 years ago

'LDheatmap' is failing in its addTracks.Rnw vignette I think because it loads system.file("extdata/addTracks.RData",package="LDheatmap"), which contains some old-style units. It should be possible to regenerate that .RData file to see if that fixes the problem (the vignette has code), but it is not working for me yet.

Did we look at trying to detect and "promote" these old-style units to new-style units at some point?

thomasp85 commented 4 years ago

Old style units should get promoted automatically now upon use, even if they are already embedded within a grob or viewport... I’ll look into this one a bit more

pmur002 commented 4 years ago

'lemon' has (in ggplot_gtable.built_lemon()) code like this ...

  do.call(grid::unit.pmax, c(widths, gtable$widths[gtable$layout$l[i]]))

... where the c() call is combining a list of units with a single unit. Goodness knows how this ever worked, but now at least that should be something like ...

  do.call(grid::unit.pmax, 
          c(widths, 
            list(gtable$widths[gtable$layout$l[i]])))

(there are 3 other instances in ggplot_gtable.built_lemon())

For the record, fixing that then exposes an error in example(geom_pointpath) "invalid line type", which appears to come from NAs in the last row of 'df' within GeomPointPath$draw_panel(), which appears to come from df <- within(munched[munched$length > threshold,] because the last value in munched$length is NA. This is too deep for me to fix.

Fortunately, this only has one reverse dependency, so there should not be a large cascade of CRAN errors.

pmur002 commented 4 years ago

'metR' has (in geom_arrow.R and slight variation in geom_streamline.R) ...

  arrow$length <- unit(as.numeric(arrow$length)*mag, attr(arrow$length, "unit"))

... which can easily be fixed with ...

  arrow$length <- mag*arrow$length

PR based on this fix sent to Elio (https://github.com/eliocamp/metR/pull/105)

This is now fixed on CRAN

pmur002 commented 4 years ago

'multipanelfigure' has ...

  round.unit <- function(x, digits = 0) {
      saved_unit <- x %>%
          attr("unit")
      x %>%
          as.numeric() %>%
          round(digits = digits) %>%
          grid::unit(saved_unit) %>%
      return()
  }

... (plus a scary-looking round.unit.list()), which is clearly peeking into unit objects.

This could be another case for the new grid::unitType() function, though I worry about the round.unit.list(); we may need to look at implementing a grid::round.unit() method ourselves to make this safe (?)

pmur002 commented 4 years ago

'NMF' fails example("NMF-package") with ...

  REAL() can only be applied to a 'numeric', not a 'integer'

... which appears to simply be REAL(values) being called on an integer (within multUnits() in unit.c).

So, the fix for this one is on us to be more defensive about either coping with both INTEGER and REAL in C code, or coercing with as.numeric() in R code.

Should be fixed now (after TLP changes to grid new units)

https://github.com/thomasp85/grid/issues/14#issuecomment-549293873

pmur002 commented 4 years ago

'ParamHelpers' is seg-faulting because grid::unit.pmax() cannot cope with NULL values (but it used to). The following code used to run (for better or worse) ...

  unit.pmax(unit(c(0, 1), c("in", "mm")), 
            unit(1, "npc"))
  unit.pmax(unit(c(0, 1), c("in", "mm")), 
            NULL, 
            unit(1, "npc"))

... but now it seg-faults (when the NULL is passed to summaryUnits() then to unitScalar(), in unit.c).

Ours to fix (by making grid::unit.pmax() more robust ?).

Should be fixed now (after TLP changes to grid new units)

https://github.com/thomasp85/grid/issues/14#issuecomment-549293873

pmur002 commented 4 years ago

'partykit' is failing with ...

  Start tag expected, '<' not found

... an 'XML' error ???

Just retest in next round.

pmur002 commented 4 years ago

'pheatmap' dies with ...

  Error in Ops.unit(m, size) : 
    REAL() can only be applied to a 'numeric', not a 'integer'

... which is just like 'NMF', so hopefully the same problem that can be solved by us being more robust about integer multipliers in x*unit().

Should be fixed now (after TLP changes to grid new units)

https://github.com/thomasp85/grid/issues/14#issuecomment-549293873

pmur002 commented 4 years ago

'Plasmidprofiler' is another ggplotly::mm2pixels() case.

pmur002 commented 4 years ago

'portfolio' fails its tests because it is trying to compare with a saved grob (which will have old-style units).

  load("map.market.test.RData")
  ...
  stopifnot(
            isTRUE(all.equal(getGrob(result, "MAP"), truth))
            )

Solution is hopefully just to regenerate saved truth.

pmur002 commented 4 years ago

'survey' failure appears to be due to 'hexbin' (see above).

pmur002 commented 4 years ago

'tmap' has ...

  unit_mod <- function(unt, a=0, b=1) {
    cls <- attr(unt, "unit")
    unit(as.numeric(unt)*b + a, cls)
  }

... which would appear to benefit from the new grid::unitType()

pmur002 commented 4 years ago

'trelliscopejs' is another ggplotly::mm2pixels() case.

pmur002 commented 4 years ago

'ufs' fails because it does not have a dependency installed ('bootES')

I will need to check whether I can install that dependency.