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

Ignoring AsIs objects (again) #5477

Merged
merged 10 commits into from
Dec 8, 2023
Merged

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Oct 12, 2023

This PR is the 3rd attempt to fix #5142 and replaces #5290 and #5418.

Briefly, instead of assigning scale_*_identity() to <AsIs> objects, it ignores these in scale operations.

Benefits of this are:

  • You can safely do aes(colour = I(...)) in one layers and do aes(colour = proper_column) in another layer, without having a clash of colour scales.
  • Using x/y = I(...) effectively makes the position aesthetic in an relative unit ("npc" units), which can be very convenient for annotations and such.

The mechanism this PR employs is to hide/unhide <AsIs> column at strategic moments during plot building.
'Ignoring' packs <AsIs> columns into one .ignored data.frame column.
'Exposing' unpacks the .ignored data.frame column into separate columns again.

A quick comparison with the other PRs:

Compared to #5290:

  • No need for scales to make a formal exemption for <AsIs> columns; plot building takes care of this.
  • For that reason, this PR has a much smaller footprint and is less invasive.

Compared to #5418:

  • Don't need bother with mappings to derive which columns should be ignored.
  • Because the <AsIs> class is 'infective', the mechanism automatically propagates through reparameterization, which is desirable (e.g. from x/width to xmin/xmax).

A quick benchmark to show that hiding/exposing scales well with dataset size (nrow = 32, 234 and 53.940 for mtcars, mpg and diamonds respectively). All in all, I think this PR wouldn't even add 1ms to plot building in 90% of plots.

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

data <- list(
  transform(mtcars, cyl = I(cyl), disp  = I(disp)),
  transform(mpg,    cyl = I(cyl), displ = I(displ)),
  transform(diamonds, color = I(color), depth = I(depth))
)

bench::mark(
  "mtcars"   = ignore_data(data[[1]]),
  "mpg"      = ignore_data(data[[2]]),
  "diamonds" = ignore_data(data[[3]]),
  "all"      = ignore_data(data), 
  check = FALSE
)
#> # A tibble: 4 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 mtcars       38.7µs   41.3µs    22758.    3.84KB     4.55
#> 2 mpg          38.1µs   40.6µs    23882.        0B     4.78
#> 3 diamonds     39.5µs   42.2µs    23048.        0B     4.61
#> 4 all         107.9µs    114µs     8594.      304B     4.08

ignored <- ignore_data(data)

bench::mark(
  "mtcars"   = expose_data(ignored[[1]]),
  "mpg"      = expose_data(ignored[[2]]),
  "diamonds" = expose_data(ignored[[3]]),
  "all"      = expose_data(ignored),
  check = FALSE
)
#> # A tibble: 4 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 mtcars       11.2µs   12.4µs    77564.        0B     7.76
#> 2 mpg          11.3µs   12.2µs    76183.        0B     0   
#> 3 diamonds     11.2µs   12.4µs    78313.        0B     7.83
#> 4 all          25.6µs   27.9µs    34930.        0B     3.49

Created on 2023-10-12 with reprex v2.0.2

R/utilities.R Outdated Show resolved Hide resolved
@teunbrand

This comment was marked as resolved.

@thomasp85
Copy link
Member

Can you update this for review?

@teunbrand teunbrand requested a review from thomasp85 December 6, 2023 09:41
@teunbrand
Copy link
Collaborator Author

I've updated this PR with the latest suggestions

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

@teunbrand
Copy link
Collaborator Author

Thanks for the review!

@teunbrand teunbrand merged commit b0e7586 into tidyverse:main Dec 8, 2023
12 checks passed
@teunbrand teunbrand deleted the ignore_asis2 branch December 8, 2023 09:46
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.

Circumventing scales
2 participants