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

Consolidate scale definition of position aesthetics #5640

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #3342 and fix #4966.

Briefly, all position scales now use the definitions of x- and y-aesthetics from the gglobal_global environment.
This ensures that rarer position aesthetics, like xintercept, contribute to the scale limits and are transformed by the scale.

I have been forewarned that this might lead to unexpected issues, but I don't foresee any at the moment. It might lead to some visual changes, but I'd argue they might be intended.

Reprex from #4966:

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

set.seed(1234)
df = tibble(
  x = as.Date(0:19, origin = ymd("2022-01-01")), 
  y = rnorm(20)
)
df |>
  ggplot(aes(x, y)) + 
  geom_point() + 
  geom_hline(yintercept = 5) +                 
  geom_vline(xintercept = ymd("2022-01-10")) +
  geom_vline(xintercept = ymd("2022-03-01"))

Created on 2024-01-12 with reprex v2.0.2

@michaelgrund
Copy link
Contributor

Are there any plans to put the solution regarding this issue into the next release? It's really annoying that vertical lines plotted far outside the date limits are not considered.

@teunbrand
Copy link
Collaborator Author

It's really annoying that vertical lines plotted far outside the date limits are not considered.

I totally agree with you there.

We want the next release to be a low-risk hotfix where we mostly fix mistakes from 3.5.0 and not put in something disruptive.
This PR has a non-zero risk of breaking the assumption of some packages that depend on ggplot2, so I think it'd be wise to postpone this a little bit further (or run revdepcheck for this PR). Any thoughts @thomasp85?

@thomasp85
Copy link
Member

yes, this will be postponed

@thomasp85
Copy link
Member

After a lot of digging I found that the addition of "intercept" to the list of positional aesthetics was the cause of the troubles I remembered. Since this is not touched here I think it's safe. Will run revdepcheck regardless

@thomasp85
Copy link
Member

Bunch of revue-check failures. I'm pretty sure that most or all are unrelated to this but please sift through them

@teunbrand
Copy link
Collaborator Author

I can see the irony in some of the revdep failures.
Basically, people have found workarounds for the problem that date scales don't accept {x/y}intercept aesthetics.
However, this PR now breaks those workarounds by making it easier.

For example this now correctly throws the error that it will not accept any non-dates.

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

# Analogous to current workarounds
ggplot(economics, aes(date, unemploy)) +
  geom_line() +
  geom_vline(xintercept = as.numeric(as.Date("1983-01-31")))
#> Error in `transformation$transform()` at ggplot2/R/scale-.R:628:3:
#> ! `transform_date()` works with objects of class <Date> only

Whereas now the solution is to simply drop the as.numeric() workaround.

ggplot(economics, aes(date, unemploy)) +
  geom_line() +
  geom_vline(xintercept = as.Date("1983-01-31"))

Created on 2024-05-21 with reprex v2.1.0

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.

geom_vline() does not extend the limits if the axis is a Date Consolidate definition of position aesthetics
3 participants