Skip to content
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

Reduce panel parameter setup in facetted plots #5431

Merged
merged 13 commits into from
Jun 25, 2024

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Sep 21, 2023

This PR aims to fix #5427.

Briefly, it determines unique combinations of x/y scales and (1) only sets up panel parameters and guides and (2) renders axes for these unique combinations. These are then repeated for the duplicated combinations.

In theory, the panel's x parameters and y parameters could be setup independently to further reduce the amount of setup is performed, but the Guide$transform method should have access to both at the same time.

Below is a reprex from the issue that shows that the x-axis is only trained and drawn once:

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

train <- 0
draw  <- 0

# Setting up a counting axis
x <- guide_axis()
x$train <- function(self, params, scale, aesthetic = NULL, ...) {
  train <<- train + 1
  GuideAxis$train(params, scale, aesthetic, ...)
}
x$draw <- function(self, theme, params) {
  draw <<- draw + 1
  GuideAxis$draw(theme, params)
}

ggplot(mpg, aes(displ, hwy)) +
  geom_point() +
  guides(x = x) +
  facet_grid(drv ~ cyl)

train
#> [1] 1
draw
#> [1] 1

Created on 2023-09-21 with reprex v2.0.2

This mostly will affect plots that have many facets but no/few unique scales. Benchmark below is for an optimal case (100 panels all sharing all scales) using this PR. All the panel param setup and drawing happens after ggplot_build(), so I'm testing the ggplot_table() performance.

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

df <- data.frame(x = rnorm(10000), y = rnorm(10000),
                 facet = sample(100, 10000, replace = TRUE))

p <- ggplot(df, aes(x, y)) +
  geom_point() +
  facet_wrap(~ facet)

build <- ggplot_build(p)

bench::mark(ggplot_gtable(build), min_iterations = 10)
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 1 × 6
#>   expression                min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>           <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 ggplot_gtable(build)    948ms    993ms     0.964    15.5MB     4.82

Created on 2023-09-21 with reprex v2.0.2

Compared to the same benchmark for the current main branch:

#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 1 × 6
#>   expression                min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>           <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 ggplot_gtable(build)    1.82s     1.9s     0.522    18.1MB     5.64

To me this is a clear performance benefit in this optimal case.

@teunbrand
Copy link
Collaborator Author

@clauswilke Do you happen to know if there is any case that coord_sf() will need to draw a different graticule per panel in a facetted plot with fixed scales? It's a tiny change to revert the behaviour in this PR for coord_sf().

@teunbrand teunbrand changed the title Coord redundancy Reduce panel parameter setup in facetted plots Sep 21, 2023
@clauswilke
Copy link
Member

If every panel uses the same projection and the same scales then the graticule should be identical. I'm only moderately confident in this statement though, haven't thought it through that carefully.

@teunbrand
Copy link
Collaborator Author

Thanks Claus! I'll take your 'moderately confident' over my 'I guess so' anytime.

@thomasp85
Copy link
Member

How will this affect extension packages with new facets? Do they need updating?

@teunbrand
Copy link
Collaborator Author

teunbrand commented Oct 2, 2023

Do they need updating?

I expect they don't need any updates. The compression is within each step but between each step the full, expanded panel parameters are available, so this shouldn't affect anything outside the methods affected.

@teunbrand
Copy link
Collaborator Author

I just realised that this would affect extensions where a RNG is used in any of the affected methods, but I expect this not to be a typical case as most plots are deterministic.

Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Merge branch 'main' into coord_redundancy

# Conflicts:
#	R/facet-grid-.R
#	R/facet-wrap.R
@teunbrand
Copy link
Collaborator Author

I didn't see it at the time writing this PR, but it conflicts with #4467.
That conflict basically means we cannot do the optimisation at the facet level, as we need separately drawn axes for interior panels versus exterior panels.
At the coord level, it takes ggplot_build(p) from ~737ms to ~23.8ms, so I'd argue that is still a worthwhile improvement (note that guide setup has moved from the gtable step to the build step in 3.5.0 after this PR was written).

@teunbrand
Copy link
Collaborator Author

Asking @thomasp85 to see if his approval still holds with the coord-level optimisation only.

@teunbrand teunbrand requested a review from thomasp85 May 20, 2024 12:09
@teunbrand
Copy link
Collaborator Author

I'm going to assume the approval still holds and go ahead with merge.

@teunbrand teunbrand merged commit c95e061 into tidyverse:main Jun 25, 2024
11 checks passed
@teunbrand teunbrand deleted the coord_redundancy branch June 25, 2024 13:51
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.

Excessive position guide training/drawing in facets
3 participants