-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Axes at interior panels #4064 #4467
Conversation
Sync branches
When I need this functionality I rely on: |
This comment has been minimized.
This comment has been minimized.
Merge branch 'master' of https://github.com/tidyverse/ggplot2 into tidyverse-master # Conflicts: # R/facet-wrap.r
Agree having this in ggplot2 will make it more mainstream I use ggh4x a lot especially the facet_nested ! Thank you ! |
@clauswilke any thoughts on this - I'm in two minds |
I like this feature. My workaround, which I use all the time, is to set scales to free and then impose scale limits. This has the same effect visually but is a bit confusing, as it requires setting two contradictory options. I'd also say that being able to omit labels for interior plots would be useful. I haven't reviewed the code, though. |
The current interface seems like a reasonable compromise to me; it meaningfully improves the display options available, without getting bogged down into too many details. However, I do think @teunbrand do you mind changing to @clauswilke do you want to review this? or should I? |
Thanks Hadley! I'll take a look in a few days time once I've got some to spare myself. |
Merge branch 'main' of https://github.com/tidyverse/ggplot2 into facet_axes # Conflicts: # R/facet-grid-.r # R/facet-wrap.r
Revisiting this topic now a while later, I don't think there currently exists a clean path towards omitting labels. We can edit the axis grob in the What I think is a neater solution would be to create an argument in the |
Merge branch 'main' into facet_axes # Conflicts: # R/facet-grid-.R # R/facet-wrap.R # man/facet_grid.Rd # tests/testthat/test-facet-.R
Yeah it looks like it should if no interior axes are drawn or the interior axes are drawn with labels. The issue is that the space reserved for axes is calculated as the maximum size for all the axes in a particular row or column. We could try to calculate the size of only the relevant axes that are in between panels, but this would get pretty gnarly and the drawing code already isn't the easiest to work with. |
Merge branch 'main' into facet_axes # Conflicts: # R/coord-cartesian-.R
Now with much better spacing for hidden labels: devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2
ggplot(mtcars, aes(mpg, disp)) +
geom_point() +
guides(x.sec = "axis", y.sec = "axis") +
facet_wrap(vars(cyl, vs), axes = "all", axis_labels = "margins") Created on 2023-10-31 with reprex v2.0.2 |
Merge branch 'main' into facet_axes # Conflicts: # R/facet-grid-.R
A small update on how this PR now interacts with newer features. When using stacked axes, only the first axis in the stack is rendered at interior panels without labels if so directed. devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2
p <- ggplot(mtcars, aes(mpg, disp)) +
geom_point() +
facet_wrap(vars(cyl, vs), axes = "all", axis_labels = "margins")
p + guides(x = guide_axis_stack("axis", "axis"),
y = guide_axis_stack("axis", "axis")) An exterior r-axis in p + coord_radial() Whereas interior r-axes are rendered in full. p + coord_radial(end = 1.5 * pi) Created on 2023-12-12 with reprex v2.0.2 |
#' interior axes get labels. When `"margins"`, only the exterior axes get | ||
#' labels and the interior axes get none. When `"all_x"` or `"all_y"`, only | ||
#' draws the labels at the interior axes in the x- or y-direction | ||
#' respectively. | ||
#' @export | ||
#' @examples | ||
#' p <- ggplot(mpg, aes(displ, cty)) + geom_point() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add an example showing the new functionality
R/facet-grid-.R
Outdated
@@ -112,7 +122,8 @@ facet_grid <- function(rows = NULL, cols = NULL, scales = "fixed", | |||
space = "fixed", shrink = TRUE, | |||
labeller = "label_value", as.table = TRUE, | |||
switch = NULL, drop = TRUE, margins = FALSE, | |||
facets = deprecated()) { | |||
facets = deprecated(), axes = "margins", | |||
axis.labels = "all") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add the new arguments in before the deprecated one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
R/facet-grid-.R
Outdated
if (params$draw_axes$x) { | ||
# Take facet_wrap approach to axis placement | ||
axes$x$top <- matrix(axes$x$top[x_axis_order], | ||
nrow = nrow, ncol = ncol, byrow = TRUE) | ||
panel_table <- weave_tables_row( | ||
panel_table, axes$x$top, -1, | ||
unit(apply(axes$x$top, 1, max_height, value_only = TRUE), "cm"), | ||
name = "axis-t", z = 3 | ||
) | ||
axes$x$bottom <- matrix(axes$x$bottom[x_axis_order], | ||
nrow = nrow, ncol = ncol, byrow = TRUE) | ||
panel_table <- weave_tables_row( | ||
panel_table, axes$x$bottom, 0, | ||
unit(apply(axes$x$bottom, 1, max_height, value_only = TRUE), "cm"), | ||
name = "axis-b", z = 3 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be abstracted away if we are using it multiple places in the code base (both for x and y and in facet_grid and facet_wrap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like this too, but the actual facet_wrap code has very involved code dealing with empty panels (restoring axes for non-empty panels and getting the correct axis sizes). This is right between the matrix(axes$...)
and weave_tables_*()
steps. We can skip all of this because facet_grid doesn't have empty panels. So while the approach is the same, it would be tricky to unify the actual code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we at least abstract it away for x and y?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see what I can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that facet_wrap()
didn't need to measure the axis before dealing with empty panels, so axis measurement and adding it to the table is now wrapped in a weave_axes()
helper. This helper is also used in facet_grid()
when required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see... I knew you could do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the review Thomas! |
This PR aims to implement drawing axes at interior facets proposed in #4064. It implements this for
facet_wrap()
as well as forfacet_grid()
. While the changes forfacet_wrap()
are quite minimal, forfacet_grid()
some additional code was borrowed fromfacet_wrap()
to draw additional axes.A few discussion points:
draw.axes
because it felt somewhat more intuitive than justaxes
to me. I'm not particularly attached to the name though.Two small demonstrations:
Created on 2021-05-05 by the reprex package (v1.0.0)