Issues with new unit implementation

[thomasp85]

• 1)

* installing *source* package ‘augmentedRCBD’ ...
...
** byte-compile and prepare package for lazy loading
Error in get(x, envir = ns, inherits = FALSE) :
 object 'unit.list' not found
Error: unable to load R code in package ‘augmentedRCBD’

which I presume should be using the generic unit().

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.

• 2)

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.

• 3)

VECTOR_ELT() can only be applied to a 'list', not a 'double'

in many packages:
BAS CINNA DescribeDisplay EMMIXmfa GGally ICtest LDheatmap
PPforest SHELF StratigrapheR airGR auditor codebook ggmcmc lemon
nzelect qualvar randomForestExplainer rrr specmine staRdom statVisual tsfeatures userfriendlyscience

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.

• 4)

 (list) object cannot be coerced to type 'double'

from unit() in ggpol.

• 5)

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

from Ops.units in IntNMF NMF SCROPIUS diceR iTOP pheatmap, seemingly all from NMF.

• 6)

`heatmap_width` and `width` should not all be the absolute units.

from CNVScope armada countToFPKM eclust wilson

• 7)

old version of unit class is no longer allowed

from fewer after reinstallation - MTLR SpatialBall firebehavioR foreSIGHT superheat xpose . Some say from is.unit().

• 8)

Error in mm2pixels(convert(u, "mm")) : Unit must be in millimeters

from MendelianRandomization Miso PCRedux PPQplan Plasmidprofiler otsad autoplotly frailtyEM getTBinR gprofiler2 heatmaply manhattanly
neatmaps otsad personalized plotly rabhit trelliscopejs wilson

• 9)
  ParamHelpers segfaults, in its obfuscated tests.

• 10)
  cowplot has a Floating point exception in shared_legends.Rmd.

• 11)

Error in FUN(X[[i]], ...) : object is not coercible to a unit
Calls: blobbogram -> Summary.unit -> unlist -> lapply -> FUN

from gemtc.

• 12)

Error in unit(3, "grobwidth", data = seg_plot_yaxis_grobs[[longest_segment]]) :
 data must be either NULL, have length 1, or match the length of the final unit vector
Calls: plot_gene_map -> unit

from genoPlotR.

• 13)

Error in grid::unit(c(0, 0, 0, marginLength), marginUnit) : Invalid unit
Calls: ggMarginal ... addMainTheme -> <Anonymous> -> find_args -> mget -> <Anonymous>

from ggExtra

• 14)

Error in `attributes<-`(x, attr) :
 'names' attribute [2] must be the same length as the vector [1]
Calls: <Anonymous> ... gtable_add_rows -> insert.unit -> unit.c -> [ -> [.unit

from ggraph

• 15)
  Error in grid::unit(c(0, 0, 0, marginLength), marginUnit) : Invalid unit
  Calls: ... addMainTheme -> -> find_args -> mget ->

from ggstatsplot

• 16)

 Error in unit(x, unit) : Invalid unit
 Calls: grid.export ... as.animUnit -> as.animUnit.numeric -> animUnit -> is.unit -> unit

from gridSVG, which I expect is known.

• 17)

Error in (function (cl, name, valueClass)  :
 c("assignment of an object of class “simpleUnit” is not valid for @‘mar’ in an object of class “hexVP”; is(value, \"unit\") is not TRUE", "assignment of an object of class “unit” is not valid for @‘mar’ in an object of class “hexVP”; is(value, \"unit\") is not TRUE", "assignment of an object of class “unit_v2” is not valid for @‘mar’ in an object of class “hexVP”; is(value, \"unit\") is not TRUE")
Calls: plot -> plot -> .local -> hexViewport -> <Anonymous>

from hexbin lidR survey

• 18)

Error in unit(as.numeric(arrow$length) * mag, attr(arrow$length, "unit")) :
 Invalid unit
Calls: <Anonymous> ... <Anonymous> -> .fun -> do.call -> <Anonymous> -> f -> unit

from metR tmap

• 19)

Warning in check_units(width, unit) :
 Multiple grid::units detected. Casting all to 'unit' argument ('mm').
Warning in check_units(height, unit) :
 Multiple grid::units detected. Casting all to 'unit' argument ('mm').
>
> # With no panels, printing shows the layout
> figure1
Error in grid::unit(., saved_unit) : Invalid unit
Calls: <Anonymous> ... eval -> _fseq -> freduce -> <Anonymous> -> <Anonymous>

from multipanelfigure

• 20)

Error in sum(g_tab$widths[[5]], g_tab$widths[[6]], g_tab$widths[[7]]) :
 invalid 'type' (list) of argument
Calls: thresh_forest

in nathresh

• 21)

 Error: isTRUE(all.equal(getGrob(result, "MAP"), truth)) is not TRUE

in portfolio

• 22)

Error in grid::unit(c(0, 0, 0, marginLength), marginUnit) : Invalid unit
Calls: scatterplot ... addMainTheme -> <Anonymous> -> find_args -> mget -> <Anonymous>

in sensitivity

• 23)

Error in matchData[[i]] * amount[i] :
 non-numeric argument to binary operator
Calls: plotSelected ... <Anonymous> -> do.call -> sisalTable -> Summary.unit

in sisal and stripless

[thomasp85]

cc @pmur002

[thomasp85]

@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]

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 ...

• installs the rcheckserver metapackage (http://statmath.wu-wien.ac.at/AASC/debian/archive-amd64/) to provide all of the system dependencies
• mounts a directory containing the R source tree with the new-unit patch applied (so that the source tree can be updated and patched, but persists across Docker containers)
• mounts a directory to build R (so that the build persists across Docker containers)
• mounts a directory of installed 'grid' direct reverse dependencies (so that the packages can be updated and persist across Docker containers)
• also mounts a directory of installed 'grid' recursive dependencies, but that is only worth running once we have things much more stable.

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]

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

[pmur002]

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

[thomasp85]

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

[pmur002]

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]

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

[pmur002]

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]

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]

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

[thomasp85]

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

[pmur002]

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

[pmur002]

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]

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?