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

Fix back-transformation of ranges in coords #2821

merged 11 commits into from Sep 25, 2018


Copy link

@clauswilke clauswilke commented Aug 10, 2018

This is a starting point for a conversation of how to fix #2149 and #2812. I realize the regression tests don't pass yet.

The key to fixing these bugs is to recognize that the range() function in Coord is meant to back-transform transformed ranges into data coordinates. It is only ever used in this way:

wilke$ grep -e '$range(' R/*
R/coord-munch.r:  ranges <- coord$range(range)
R/geom-abline.r:    ranges <- coord$range(panel_params)
R/geom-hline.r:    ranges <- coord$range(panel_params)
R/geom-vline.r:    ranges <- coord$range(panel_params)

and the API only makes sense with this interpretation: The function takes panel ranges as argument and returns ranges. If it doesn't back-transform, what else should it do? Also, it is written in this way for coord_flip() and coord_polar() already.

To make the meaning of the function more explicit, I have renamed it from range() to backtransform_range(). However, this is a breaking change if any geoms out in the world use this function. Alternatively, we could keep the current (confusing) name and just document better.

Reprex with fixed examples follows below. These geoms don't work at all yet with coord_sf(), but there appears to be a separate issue that needs to be fixed first (#2820).


# original example from #2149
df <- data.frame(x=1:6, y=10^(1:6))
ggplot(df, aes(x, y)) +
  geom_point() +
  geom_vline(xintercept=3, color='red') +

# original example from #2812
mydf <- data.frame(x = 0L:20,
                   y = -10:10)
ggplot(data = mydf, aes(x, y)) + 
  geom_point() + 
  coord_trans(x = "log1p") +
  geom_hline(yintercept = c(-10, 10))

# original example from #2340
log_modulus_trans <- function() 
  trans_new(name = "log_modulus", 
            transform = function(x) sign(x) * log(abs(x) + 1), 
            inverse = function(x) sign(x) * ( exp(abs(x)) - 1 ))

d <- data_frame(
  tt = rep(1:10, 3),
  cc = rep(LETTERS[1:3], each = 10),
  xx = c(rnorm(10, mean = 100, sd = 10), 
         rnorm(10, mean = 0, sd = 10),
         rnorm(10, mean = -100, sd = 10)))

ggplot(data = d, 
       mapping = aes(x = tt, y = xx, group = cc)) +
  geom_line() + 
  geom_vline(xintercept = 5) +
  coord_trans(y = "log_modulus")

# examples with geom_line(), from #2149
df <- data.frame(x = c(-Inf, Inf), y = c(1, 2))
p <- ggplot(df, aes(x, y)) + geom_line() + xlim(1, 2)
p + coord_polar()

p + coord_trans(y = "log10")

p + coord_trans(x = "log10")

Created on 2018-08-10 by the reprex package (v0.2.0).

Copy link
Member Author

@clauswilke clauswilke commented Aug 10, 2018

@hadley Could you comment on the naming choice of the range() function (keep name or change to something like backtransform_range())? I can then proceed accordingly and fix the regression tests and #2820.

Copy link

@hadley hadley left a comment

I think changing the name is probably ok; we'll just need to remember to call out in the NEWS for the next release (since it's a breaking change for extension authors) (but I don't think there are many custom coords in the wild)

Can you please also add some text to the Coord help?

R/coord-.r Outdated
# back into range in given in (possibly scale-transformed)
# data coordinates
backtransform_range = function(panel_params) {
warning("range backtransformation not implemented in this coord; plot may be wrong.")

This comment has been minimized.


hadley Aug 10, 2018

Needs call. = FALSE

@clauswilke clauswilke force-pushed the clauswilke:issue-2149-range-backtransform branch 2 times, most recently from 5ccca17 to 17822b5 Aug 10, 2018
Copy link
Member Author

@clauswilke clauswilke commented Aug 11, 2018

I have improved the coord documentation, implemented backtransform_range() for all coords, implemented a deprecated range() function that prevents the code of any potential downstream users from breaking, and fixed the regression tests.

Copy link
Member Author

@clauswilke clauswilke commented Aug 17, 2018

All fixes and documentation improvements minus the API change are now in PR #2832, which I think should be merged first. Once that is done I will rebase this PR.

Copy link

@hadley hadley commented Aug 17, 2018

I made a new label to (hopefully) remind us not to merge until we're done with the bug fix release.

@clauswilke clauswilke force-pushed the clauswilke:issue-2149-range-backtransform branch from 17822b5 to f094a43 Aug 23, 2018
schloerke added a commit to schloerke/shiny that referenced this pull request Sep 16, 2018
…ersion of ggplot2

Originally caused by tidyverse/ggplot2#2832

Need to wait for tidyverse/ggplot2#2821 to be merged.

Expecting a bug fix release between 3.0.0 and when this bug will be fixed, so can not compare against pkg versions. :-/
Copy link

@wch wch commented Sep 17, 2018

Is it possible to get this in the bug-fix release? The API change from #2832 causes a problem with plot interaction in Shiny, and this PR would fix it. We'd prefer not to add in a workaround for a single version of ggplot2 that has a different API.

(@schloerke Please feel free to correct me if I've gotten details wrong.)

Copy link
Member Author

@clauswilke clauswilke commented Sep 17, 2018

@wch @schloerke Could you file a separate ggplot2 issue explaining exactly what the problem is? I looked at rstudio/shiny#2184 and it's not immediately clear to me why this PR would fix the problem. More fundamentally, I'm concerned that there's a problem with the current ggplot2 that would be resolved with this PR. It suggests to me we haven't fully thought this through.

@clauswilke clauswilke force-pushed the clauswilke:issue-2149-range-backtransform branch from 56bc199 to d4be2f1 Sep 18, 2018
Copy link
Member Author

@clauswilke clauswilke commented Sep 18, 2018

@hadley Could you review this once more? I think this is now as much as possible a non-breaking commit:

  • The range() function behaves as before for all coords.
  • The summarise_layout() function behaves as before for all coords.
  • The new backtransform_range() function uses range() as fallback (with warning) if a custom coord does not implement it.

With these changes, I think it is better to merge this before 3.0.1 than not, since it fixes #2895.

I have also provided documentation for the summarise_layout() function so in the future we will know what it does.

@clauswilke clauswilke mentioned this pull request Sep 18, 2018
25 tasks done
hadley approved these changes Sep 25, 2018
Copy link

@hadley hadley left a comment

I'm not very familiar with this code any more, but the changes seem reasonable to me, and the documentation looks good.

@clauswilke clauswilke merged commit 4af1337 into tidyverse:master Sep 25, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 77.81%)
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +22.18% compared to 484a220
continuous-integration/appveyor/pr AppVeyor build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
@clauswilke clauswilke deleted the clauswilke:issue-2149-range-backtransform branch Sep 25, 2018
Copy link

@lock lock bot commented Mar 24, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants