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

Implement n_breaks in continuous_scale #3102

Merged
merged 7 commits into from Nov 14, 2019

Conversation

thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Jan 25, 2019

This is following thoughts and discussion in #3096 and implement an n_breaks argument in continuous_scale() to allow guiding the number of major breaks that gets created. Before you'd had to make the breaks manually if you wanted to have a different number of breaks (or construct your own trans object by hand)...

The implementation can feel a bit hacky as it modifies the parent environment of the breaks method of the trans object. I make sure to reset the parent environment on exit so there should be no side effects, but a more elegant approach is if scales allowed dynamic setting of n in trans$breaks()

Anyway, consider this PR as discussion ground for the feature and we can figure out the best implementation later if we want it...

example use:

library(ggplot2)

ggplot(mtcars) + 
  geom_point(aes(mpg, disp))

ggplot(mtcars) + 
  geom_point(aes(mpg, disp)) + 
  scale_x_continuous(n_breaks = 10)

Created on 2019-01-25 by the reprex package (v0.2.1)

@hadley
Copy link
Member

@hadley hadley commented Jan 25, 2019

Can't we implement this by setting breaks to a function?

@thomasp85
Copy link
Member Author

@thomasp85 thomasp85 commented Jan 25, 2019

Indeed, but I think having a direct argument accepting an integer will make users actually use it... having to construct a function that wraps something from the labelling package seems a high bar for such a simple need (IMO)

@thomasp85
Copy link
Member Author

@thomasp85 thomasp85 commented Jan 25, 2019

Or do you mean that we should do this under the hood during construction of the scale?

@thomasp85
Copy link
Member Author

@thomasp85 thomasp85 commented Jan 25, 2019

I think it is going to be hard to implement under the hood as functions generally because different transformations have different breaks functions, so we'd have to reverse engineer these... this is the reason why I've currently accepted the approach implemented here

@hadley
Copy link
Member

@hadley hadley commented Jan 25, 2019

If you think this is really important, then I think you need to start by refactoring the existing breaks code to make it easier to pass n_breaks along to the right place. I don't think hacking into the environment like this is a good idea.

@thomasp85
Copy link
Member Author

@thomasp85 thomasp85 commented Jan 25, 2019

I don’t think that it is super important, but it needs to be figured out for scale_binned anyway where it’ll be an important argument. I’d much rather fix this in scales, so if you’d accept a fix there I’d happily provide one

@hadley
Copy link
Member

@hadley hadley commented Jan 25, 2019

Yeah, I'd happily accept a fix in scales

@thomasp85 thomasp85 requested a review from hadley Oct 1, 2019
R/scale-.r Outdated
breaks = waiver(), minor_breaks = waiver(), n_breaks = NULL, labels = waiver(),
limits = NULL, rescaler = rescale, oob = censor, expand = waiver(), na.value = NA_real_,
trans = "identity", guide = "legend", position = "left", super = ScaleContinuous) {
ScaleBinned <- ggproto("ScaleBinned", Scale,
Copy link
Member

@hadley hadley Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with the big diff here?

Copy link
Member Author

@thomasp85 thomasp85 Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must be looking at the code from before I merged master in? scale-.r was reorganised while this PR was developed

hadley
hadley approved these changes Oct 1, 2019
@thomasp85 thomasp85 merged commit 5e388e1 into tidyverse:master Nov 14, 2019
2 of 4 checks passed
@lock
Copy link

@lock lock bot commented May 20, 2020

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. https://reprex.tidyverse.org/

1 similar comment
@lock
Copy link

@lock lock bot commented May 20, 2020

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. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants