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 coordinate scale expansion and limiting code #3380

Merged

Conversation

@paleolimbot
Copy link
Collaborator

commented Jun 23, 2019

This is a PR to address coordinate scale expansion and limiting using a single approach, implementing several feature requests and fixing several bugs in the process (closes #2990, closes #2907, closes #3056, closes #3336). The inspiration for this PR was a collection of issues that @hadley collected when issue triaging that were related to the interface between scales and coordinate systems (#3371). I'd be happy to split this up into a few different PRs if it is easier to review (one for expansion, one for coord limiting, one for sprucing up coord_trans).

  • All scale expansion code now lives in scale-expansion.r, and is tested independently of the scales/coords. All Coord objects now use these expansion functions. Previously, some form of scale expansion code lived in utilities.r, scale-.r, coord-sf.r, coord-cartesian.r, scale-discrete.r, and coord-transform.r. This led to at least one issue of inconsistent expansion between coords (#3338), and was thought to cause several other issues, although it turns out that #2990 was unrelated, #3270 had already been fixed before this PR, and nothing about this PR addressed #980.

  • All coord_*() functions with xlim and ylim arguments now accept vectors with NA as a placeholder for the minimum or maximum value (e.g., ylim = c(0, NA) would zoom the y-axis from 0 to the maximum value observed in the data). This mimics the behaviour of the limits argument in continuous scale functions (#2907).

library(ggplot2)
ggplot(mpg, aes(cty, hwy)) + 
  geom_point() +
  coord_cartesian(xlim = c(20, NA), ylim = c(NA, 40))

  • coord_trans() now draws second axes (#2990) and accepts xlim, ylim, and expand arguments to bring it up to feature parity with coord_cartesian(). This also closes #3056, which is an abandoned PR that deprecated limx and limy in favour of xlim and ylim. This PR does the same, and also fully removes xtrans and ytrans, which have been deprecated long enough that using them resulted in an error anyway.

  • coord_trans() now calculates breaks using the expanded range (previously these were calculated using the unexpanded range, which resulted in differences between plots made with coord_trans() and those made with coord_cartesian(). The expansion for discrete axes in coord_trans() was also updated such that it behaves identically to that in coord_cartesian() (see #3338 - this was already closed by the OP, but it is the best discussion of this topic).

library(ggplot2)

p <- ggplot(mpg, aes(class, hwy)) + geom_boxplot() 

# these two plots didn't used to align and sometimes had
# different breaks on the y-axis
p + scale_y_log10()
p + 
  coord_trans(y = "log10") + 
  scale_y_continuous(breaks = scales::log_breaks())
  • Added regression tests for coord_trans(), for which previously there were none.

@paleolimbot paleolimbot requested a review from hadley Jun 24, 2019

Show resolved Hide resolved R/axis-secondary.R
Show resolved Hide resolved R/coord-sf.R Outdated
Show resolved Hide resolved R/coord-transform.r
Show resolved Hide resolved R/scale-expansion.r Outdated
Show resolved Hide resolved R/scale-expansion.r Outdated
Show resolved Hide resolved R/scale-expansion.r
Show resolved Hide resolved R/scale-expansion.r Outdated
Show resolved Hide resolved R/scale-expansion.r Outdated
@hadley

hadley approved these changes Jul 1, 2019

Show resolved Hide resolved R/scale-expansion.r Outdated

@paleolimbot paleolimbot merged commit 7f317d4 into tidyverse:master Jul 1, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@paleolimbot paleolimbot deleted the paleolimbot:issue-3371-expantion-strategy branch Jul 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.