From 9388b57e7b11b971a6a05303129ea6795029131d Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 17 Nov 2021 11:59:30 +0100 Subject: [PATCH 1/4] New approach to adding back missing axes --- NEWS.md | 3 ++ R/facet-wrap.r | 84 +++++++++++++++++++++++++++++++++++++------------- 2 files changed, 65 insertions(+), 22 deletions(-) diff --git a/NEWS.md b/NEWS.md index 9a8a8fc713..3648db21db 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # ggplot2 (development version) +* Axes are now added correctly in `facet_wrap()` when `as.table = TRUE` + (@thomasp85, #4553) + * Strip padding in `facet_grid()` is now only in effect if `strip.placement = "outside"` _and_ an axis is present between the strip and the panel (@thomasp85, #4610) diff --git a/R/facet-wrap.r b/R/facet-wrap.r index 47bf1f471f..e6f34cd7af 100644 --- a/R/facet-wrap.r +++ b/R/facet-wrap.r @@ -313,30 +313,70 @@ FacetWrap <- ggproto("FacetWrap", Facet, ) # Add back missing axes if (any(empties)) { - first_row <- which(apply(empties, 1, any))[1] - 1 - first_col <- which(apply(empties, 2, any))[1] - 1 - row_panels <- which(layout$ROW == first_row & layout$COL > first_col) - row_pos <- convertInd(layout$ROW[row_panels], layout$COL[row_panels], nrow) - row_axes <- axes$x$bottom[layout$SCALE_X[row_panels]] - col_panels <- which(layout$ROW > first_row & layout$COL == first_col) - col_pos <- convertInd(layout$ROW[col_panels], layout$COL[col_panels], nrow) - col_axes <- axes$y$right[layout$SCALE_Y[col_panels]] + row_ind <- row(empties) + col_ind <- col(empties) inside <- (theme$strip.placement %||% "inside") == "inside" - if (params$strip.position == "bottom" && - !inside && - any(!vapply(row_axes, is.zero, logical(1))) && - !params$free$x) { - warn("Suppressing axis rendering when strip.position = 'bottom' and strip.placement == 'outside'") - } else { - axis_mat_x_bottom[row_pos] <- row_axes + empty_bottom <- apply(empties, 2, function(x) c(diff(x) == 1, FALSE)) + if (any(empty_bottom)) { + pos <- which(empty_bottom) + panel_loc <- data_frame(ROW = row_ind[pos], COL = col_ind[pos]) + # Substitute with vctrs::vec_match(panel_loc, layout[, c("ROW", "COL")]) + # Once we switch to vctrs wholesale + panels <- merge(panel_loc, cbind(layout, .index = seq_len(nrow(layout))))$.index + x_axes <- axes$x$bottom[layout$SCALE_X[panels]] + if (params$strip.position == "bottom" && + !inside && + any(!vapply(x_axes, is.zero, logical(1))) && + !params$free$x) { + warn("Suppressing axis rendering when strip.position = 'bottom' and strip.placement == 'outside'") + } else { + axis_mat_x_bottom[pos] <- x_axes + } } - if (params$strip.position == "right" && - !inside && - any(!vapply(col_axes, is.zero, logical(1))) && - !params$free$y) { - warn("Suppressing axis rendering when strip.position = 'right' and strip.placement == 'outside'") - } else { - axis_mat_y_right[col_pos] <- col_axes + empty_top <- apply(empties, 2, function(x) c(FALSE, diff(x) == -1)) + if (any(empty_top)) { + pos <- which(empty_top) + panel_loc <- data_frame(ROW = row_ind[pos], COL = col_ind[pos]) + panels <- merge(panel_loc, cbind(layout, .index = seq_len(nrow(layout))))$.index + x_axes <- axes$x$top[layout$SCALE_X[panels]] + if (params$strip.position == "top" && + !inside && + any(!vapply(x_axes, is.zero, logical(1))) && + !params$free$x) { + warn("Suppressing axis rendering when strip.position = 'top' and strip.placement == 'outside'") + } else { + axis_mat_x_top[pos] <- x_axes + } + } + empty_right <- t(apply(empties, 1, function(x) c(diff(x) == 1, FALSE))) + if (any(empty_right)) { + pos <- which(empty_right) + panel_loc <- data_frame(ROW = row_ind[pos], COL = col_ind[pos]) + panels <- merge(panel_loc, cbind(layout, .index = seq_len(nrow(layout))))$.index + y_axes <- axes$y$right[layout$SCALE_Y[panels]] + if (params$strip.position == "right" && + !inside && + any(!vapply(y_axes, is.zero, logical(1))) && + !params$free$y) { + warn("Suppressing axis rendering when strip.position = 'right' and strip.placement == 'outside'") + } else { + axis_mat_y_right[pos] <- y_axes + } + } + empty_left <- t(apply(empties, 1, function(x) c(FALSE, diff(x) == -1))) + if (any(empty_left)) { + pos <- which(empty_left) + panel_loc <- data_frame(ROW = row_ind[pos], COL = col_ind[pos]) + panels <- merge(panel_loc, cbind(layout, .index = seq_len(nrow(layout))))$.index + y_axes <- axes$y$left[layout$SCALE_Y[panels]] + if (params$strip.position == "left" && + !inside && + any(!vapply(y_axes, is.zero, logical(1))) && + !params$free$y) { + warn("Suppressing axis rendering when strip.position = 'left' and strip.placement == 'outside'") + } else { + axis_mat_y_left[pos] <- y_axes + } } } panel_table <- weave_tables_row(panel_table, axis_mat_x_top, -1, axis_height_top, "axis-t", 3) From bf846f5dcd9801a64bfcac9aaf894891d5cff49f Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 17 Nov 2021 11:59:40 +0100 Subject: [PATCH 2/4] add visual tests --- ...e-positioned-correctly-in-table-layout.svg | 504 ++++++++++++++++++ tests/testthat/test-facet-layout.r | 8 + 2 files changed, 512 insertions(+) create mode 100644 tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-table-layout.svg diff --git a/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-table-layout.svg b/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-table-layout.svg new file mode 100644 index 0000000000..6af5015544 --- /dev/null +++ b/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-table-layout.svg @@ -0,0 +1,504 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +midsize + + + + + + + + + + +subcompact + + + + + + + + + + + + + + + + + + + +compact + + + + + + + + + + +pickup + + + + + + + + + + + + + + + + + + + +2seater + + + + + + + + + + +minivan + + + + + + + + + + +suv + + + + + + + + +2 +3 +4 +5 +6 +7 + + + + + + +2 +3 +4 +5 +6 +7 + + + + + + +2 +3 +4 +5 +6 +7 + + + +20 +30 +40 + + + +20 +30 +40 + + + +20 +30 +40 +displ +hwy +Axes are positioned correctly in table layout + + diff --git a/tests/testthat/test-facet-layout.r b/tests/testthat/test-facet-layout.r index b975135557..7425535298 100644 --- a/tests/testthat/test-facet-layout.r +++ b/tests/testthat/test-facet-layout.r @@ -80,6 +80,14 @@ test_that("wrap: as.table reverses rows", { expect_equal(two$ROW, c(1, 1)) }) +test_that("wrap: as.table gets axes", { + p <- ggplot(mpg, aes(displ, hwy)) + + geom_point() + + scale_y_continuous(position = 'right') + + facet_wrap(vars(class), dir = "v", as.table = TRUE) + expect_doppelganger("Axes are positioned correctly in table layout", p) +}) + test_that("grid: as.table reverses rows", { one <- panel_layout(facet_grid(a~., as.table = FALSE), list(a)) expect_equal(as.character(one$a), c("2", "1")) From 9aee06134b58fa0f58f457208401077be51fa6de Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 24 Nov 2021 09:28:52 +0100 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Hiroaki Yutani --- NEWS.md | 2 +- tests/testthat/test-facet-layout.r | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/NEWS.md b/NEWS.md index 3648db21db..f15a4204d5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,6 @@ # ggplot2 (development version) -* Axes are now added correctly in `facet_wrap()` when `as.table = TRUE` +* Axes are now added correctly in `facet_wrap()` when `as.table = FALSE` (@thomasp85, #4553) * Strip padding in `facet_grid()` is now only in effect if `strip.placement = "outside"` diff --git a/tests/testthat/test-facet-layout.r b/tests/testthat/test-facet-layout.r index 7425535298..e701a16915 100644 --- a/tests/testthat/test-facet-layout.r +++ b/tests/testthat/test-facet-layout.r @@ -83,8 +83,8 @@ test_that("wrap: as.table reverses rows", { test_that("wrap: as.table gets axes", { p <- ggplot(mpg, aes(displ, hwy)) + geom_point() + - scale_y_continuous(position = 'right') + - facet_wrap(vars(class), dir = "v", as.table = TRUE) + scale_y_continuous(position = "right") + + facet_wrap(vars(class), dir = "v", as.table = FALSE) expect_doppelganger("Axes are positioned correctly in table layout", p) }) From 34d483fa7cd10c766bedd8abd93434005af22c17 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 24 Nov 2021 09:36:38 +0100 Subject: [PATCH 4/4] update tests to actually test what this is fixing --- ...sitioned-correctly-in-non-table-layout.svg | 504 ++++++++++++++++++ ...e-positioned-correctly-in-table-layout.svg | 504 ------------------ tests/testthat/test-facet-layout.r | 6 +- 3 files changed, 507 insertions(+), 507 deletions(-) create mode 100644 tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-non-table-layout.svg delete mode 100644 tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-table-layout.svg diff --git a/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-non-table-layout.svg b/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-non-table-layout.svg new file mode 100644 index 0000000000..675d65dd25 --- /dev/null +++ b/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-non-table-layout.svg @@ -0,0 +1,504 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +subcompact + + + + + + + + + + +midsize + + + + + + + + + + + + + + + + + + + +pickup + + + + + + + + + + +compact + + + + + + + + + + +suv + + + + + + + + + + +minivan + + + + + + + + + + +2seater + + + + + + + + +2 +3 +4 +5 +6 +7 + + + + + + +2 +3 +4 +5 +6 +7 + + + + + + +2 +3 +4 +5 +6 +7 +20 +30 +40 + + + +20 +30 +40 + + + +20 +30 +40 + + + +displ +hwy +Axes are positioned correctly in non-table layout + + diff --git a/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-table-layout.svg b/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-table-layout.svg deleted file mode 100644 index 6af5015544..0000000000 --- a/tests/testthat/_snaps/facet-layout/axes-are-positioned-correctly-in-table-layout.svg +++ /dev/null @@ -1,504 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -midsize - - - - - - - - - - -subcompact - - - - - - - - - - - - - - - - - - - -compact - - - - - - - - - - -pickup - - - - - - - - - - - - - - - - - - - -2seater - - - - - - - - - - -minivan - - - - - - - - - - -suv - - - - - - - - -2 -3 -4 -5 -6 -7 - - - - - - -2 -3 -4 -5 -6 -7 - - - - - - -2 -3 -4 -5 -6 -7 - - - -20 -30 -40 - - - -20 -30 -40 - - - -20 -30 -40 -displ -hwy -Axes are positioned correctly in table layout - - diff --git a/tests/testthat/test-facet-layout.r b/tests/testthat/test-facet-layout.r index e701a16915..bf99bbec00 100644 --- a/tests/testthat/test-facet-layout.r +++ b/tests/testthat/test-facet-layout.r @@ -80,12 +80,12 @@ test_that("wrap: as.table reverses rows", { expect_equal(two$ROW, c(1, 1)) }) -test_that("wrap: as.table gets axes", { +test_that("wrap: as.table = FALSE gets axes", { p <- ggplot(mpg, aes(displ, hwy)) + geom_point() + - scale_y_continuous(position = "right") + + scale_y_continuous(position = "left") + facet_wrap(vars(class), dir = "v", as.table = FALSE) - expect_doppelganger("Axes are positioned correctly in table layout", p) + expect_doppelganger("Axes are positioned correctly in non-table layout", p) }) test_that("grid: as.table reverses rows", {