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

Avoid theme partial string matching #5335

Closed
wants to merge 3 commits into from

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Jun 25, 2023

This PR aims to fix #2724.

Briefly, it replaces most instances of theme$setting with calc_element("setting", theme).
A particularly gnarly case was guide_legend() that overrides inheritance, but I've tried to use double-bracket indexing in such cases. Also updated vignette with this, hopefully encouraging people to use calc_element() instead.

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.

calc_element() is a pretty meaty function to substitute in "just" to avoid partial matching... I'm a bit concerned about the impact on performance of this

R/facet-grid-.R Outdated
theme$panel.spacing.x %||% theme$panel.spacing)
calc_element("panel.spacing.x", theme))
Copy link
Member

Choose a reason for hiding this comment

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

we don't really use this indentation style in the codebase

@teunbrand
Copy link
Collaborator Author

It is a about 2 milliseconds slower (out of 103/105 ms) on a facetted plot with 3 guides, which isn't too terrible I suppose.
Benchmarks in details below.

With the current main branch:

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

p <- ggplot(mtcars, aes(disp, mpg, colour = drat, fill = wt, size = cyl)) +
  geom_point(shape = 21) +
  facet_wrap(~ factor(gear))
b <- ggplot_build(p)

bench::mark(ggplot_gtable(b), iterations = 20)
#> # 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(b)    101ms    103ms      9.74    7.39MB     7.31

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

With this PR:

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

p <- ggplot(mtcars, aes(disp, mpg, colour = drat, fill = wt, size = cyl)) +
  geom_point(shape = 21) +
  facet_wrap(~ factor(gear))
b <- ggplot_build(p)

bench::mark(ggplot_gtable(b), iterations = 20)
#> # 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(b)    103ms    105ms      9.54    7.42MB     7.16

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

@teunbrand
Copy link
Collaborator Author

Closing this in favour of #5522

@teunbrand teunbrand closed this Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

R partial string matching can have unexpected results when theme elements are missing
2 participants