Skip to content

Fix index out of bounds in simplify_gt.gtable_patchwork() - #472

Open
frvirard wants to merge 1 commit into
thomasp85:mainfrom
frvirard:fix/simplify-gt-height-typo
Open

Fix index out of bounds in simplify_gt.gtable_patchwork()#472
frvirard wants to merge 1 commit into
thomasp85:mainfrom
frvirard:fix/simplify-gt-height-typo

Conversation

@frvirard

Copy link
Copy Markdown

Summary

simplify_gt.gtable_patchwork() computes the collapsed height of a nested
patchwork from gt$widths[panel_pos$t:panel_pos$b] instead of
gt$heights[panel_pos$t:panel_pos$b]:

if (all(is_abs_unit(gt$heights[panel_pos$t:panel_pos$b]))) {
  new_height <- sum(convertHeight(gt$widths[panel_pos$t:panel_pos$b], 'mm'))
  #                                ^^^^^^^ should be gt$heights
} else {
  new_height <- unit(1, 'null')
}

gt$widths only has one entry per column, so as soon as a nested block's
row range (panel_pos$t:panel_pos$b) exceeds the table's column count -
which happens whenever every height in the nested layout is an absolute
unit (unit(x, "cm")/"pt"/"in", i.e. is_abs_unit() is TRUE for all
of them) - the index goes out of bounds and printing/building the
patchwork fails with:

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

Using a context-relative unit (e.g. "line") for at least one height
avoids the bug, since is_abs_unit() then returns FALSE and the safe
else branch is taken instead - that's how I traced it back to this line.
Introduced in 556874d ("General solution to inheriting dimensions from
plot/table").

Reproducible example

library(ggplot2)
library(patchwork)
library(grid)

make_block <- function(n_lines) {
  title <- textGrob(paste(rep("label line", n_lines), collapse = "\n"))
  plot  <- ggplot(mtcars, aes(mpg, wt)) + geom_point()
  wrap_elements(title) + plot +
    plot_layout(ncol = 1, heights = unit.c(unit(n_lines, "cm"), unit(5, "cm")))
}

combined <- wrap_plots(list(make_block(1), make_block(3)), ncol = 1)
ggsave("reprex.pdf", combined, width = 5, height = 8)
#> Error: index out of bounds ("unit" subsetting)

Changes

  • R/plot_patchwork.R: one-line fix, gt$widthsgt$heights in the
    new_height branch.
  • tests/testthat/test-layout.R: regression test reproducing the crash
    above and asserting patchworkGrob() no longer errors on it.
  • NEWS.md: changelog entry.

Test plan

  • Confirmed the added test fails against unpatched patchwork 1.3.2
    and passes after the fix (verified locally, both via a direct
    patchworkGrob() call and via testthat::test_file()).
  • Ran tests/testthat/test-layout.R with pkgload::load_all(); no
    failures (pre-existing vdiffr snapshot tests skip in this
    environment, unrelated to this change).

new_height was computed from gt$widths[panel_pos$t:panel_pos$b] instead
of gt$heights[panel_pos$t:panel_pos$b]. gt$widths only has one entry per
column, so as soon as a nested block's row range exceeded its column
count - which happens whenever every height in the nested layout is an
absolute unit - the index went out of bounds and threw "index out of
bounds ('unit' subsetting)".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant