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

Minimal PR for unit support in positional aesthetics #5690

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mjskay
Copy link
Contributor

@mjskay mjskay commented Feb 11, 2024

Following discussion on #5609 and helpful feedback from @teunbrand, @pmur002, and @thomasp85 (thanks!), this is a second proof-of-concept PR for unit support in positional aesthetics in ggplot2. This PR focuses on being a minimal implementation without hacking on the unit type system.

The basic idea here is to instead add another aesthetic evaluation stage, after_coord, which is akin to after_stat or after_scale. It is implemented by passing aesthetic mappings with stage() or after_coord() through to Coord$transform() via the panel_params argument, which is already passed through to that function. Coord$transform() can then apply the after_coord mappings after it does its own Coord-specific transformations.

Some demos:

data.frame(var1 = 1:5, var2 = 1:5, name = letters[1:5]) |>
  ggplot(aes(var1, var2)) + 
  geom_point() +
  geom_line() +
  # labels exactly 8 points left and 6 points above their points, no matter how 
  # the plot is resized
  geom_text(aes(
    label = name, 
    x = stage(var1, after_coord = unit(x, "native") - unit(8, "pt")),
    y = stage(var2, after_coord = unit(y, "native") + unit(6, "pt"))
  )) +
  # an annotation that is always 10 points inset from the lower right
  # N.B. can't use annotate() because it doesn't understand stages, but using a 
  # geom with data = data.frame() instead seems to work. If desired, I believe
  # annotate() could be made to understand stages by having it check for the
  # stage / after_stat / after_scale /after_coord functions in the bare
  # expressions supplied to it.
  geom_text(
    aes(
      x = after_coord(unit(1, "npc") - unit(10, "pt")),
      y = after_coord(unit(10, "pt"))
    ),
    label = "some label", vjust = 0, hjust = 1,
    data = data.frame()
  ) +
  # a horizontal line exactly 25 points from the bottom.
  # can't use after_coord() with the yintercept aesthetic because geom_hline()
  # translates that to `y` and `yend` under the hood before the after_coord()
  # stage, so must modify those values directly
  geom_hline(aes(
    yintercept = Inf,  # placeholder value to avoid "missing required aes" error
    y = after_coord(unit(25, "pt")),
    yend = after_coord(unit(25, "pt")),
  ))

image

With polar coordinates:

data.frame(var1 = 1:5, var2 = 1:5, name = letters[1:5]) |>
  ggplot(aes(var1, var2)) + 
  geom_point() +
  geom_line() +
  geom_text(aes(
    label = name, 
    x = stage(var1, after_coord = unit(x, "native") + unit(10, "pt"))
  )) +
  geom_text(
    aes(
      x = after_coord(unit(1, "npc") - unit(10, "pt")),
      y = after_coord(unit(10, "pt"))
    ),
    label = "some label", vjust = 0, hjust = 1,
    data = data.frame()
  ) +
  coord_polar()

image

Some notes:

  • The main workhorse here is compute_staged_aes(), which is based on a chunk of code I factored out of Geom$use_defaults() for computing after_scale aesthetics.

  • after_coord() is a bit different from after_scale() in that it provides the placeholder value Inf to earlier parts of the pipeline, because it needed a non-unit() value that would not affect scales (see the comment on that function). There should probably be a better way to do this.

  • the implementation moves current Coord$transform() implementations into Coord$transform_numeric(). This was done so that Geoms (which already call Coord$transform()) would not have to opt in to supporting unit()s; instead, Coords in extension packages can opt-in simply by renaming their transform() methods to transform_numeric(). An alternative might be to leave Coord$transform() as-is but add a Coord$transform_unit() method, and then have Geoms opt-in to unit support instead of having Coords opt-in to it. I think having Coords opt-in is better since it requires fewer changes in extension packages and propagates support more easily, but I could be wrong.

  • No hacking on unit() is required for this implementation, nor any upstream changes to {grid}. A small set of compatibility functions would need to be added to {vctrs}, akin to how {vctrs} supplies compatibility functions for other base-R types, like Dates. These are confined to R/utilities-unit.R, which could be turned into a PR on {vctrs}.

  • In exchange for less "magic", this approach is a little more verbose than my original proposal and requires a little more understanding of the ggplot2 pipeline for users. Something like this in the current proposal:

    x = stage(var1, after_coord = unit(x, "native") - unit(8, "pt"))

    could be written as this in the original proposal:

    x = var1 - ggunit(8, "pt")

    However, this new proposal has more consistent semantics and no heuristic hacking of the unit() datatype :). A ggunit() subtype of unit() with consistent coercion rules for numerics could still be created to improve the syntax a bit (also without heuristic hacking of unit()); allowing something like this to work:

    x = stage(var1, after_coord = x - ggunit(8, "pt"))

    Adding shortcuts like as_pt() could shorten this still, to:

    x = stage(var1, after_coord = x - as_pt(8))

    The ggunit() subtype could be added to ggplot2, or if that is not desired, I'd be happy to create a small extension package with that capability.

@mjskay
Copy link
Contributor Author

mjskay commented Nov 20, 2024

@teunbrand I wanted to circle back around here at some point and see if there's still potential for this.

I think a principled inclusion of unit support would be very cool and help improve a lot of stuff at the annotation level. This PR isn't all the way there, but is much less hacky than the previous version, and I'd be happy to iterate on it if there's interest.

@teunbrand
Copy link
Collaborator

I'm happy to help think about the code and I still like the concept a lot. It is great that this PR is much less invasive than the previous one.

However I feel like the sheer scope of this PR is outside my jurisdiction and the decision on whether to forge ahead with this is in @thomasp85's hands. If he doesn't reply here after invoking his name, I'll be sure to bring it up the next time Thomas and I have a chat (in ~2 weeks time).

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.

2 participants