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

Change minor_breaks interface #3591

Closed
wants to merge 2 commits into from
Closed

Change minor_breaks interface #3591

wants to merge 2 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented Oct 27, 2019

To pass in both limits and major breaks. This will make it much easier to create useful families of minor break functions in scales.

Fixes #3583

@thomasp85 — if you think this approach is reasonable I'll work on a NEWS bullet. I think documentation will need to happen in a separate PR because all of breaks, labels, and minor_breaks need revision to point to the reader towards the scales package.

To pass in both limits and major breaks. This will make it much easier to create useful families of minor break functions in scales.

Fixes #3583
@hadley
Copy link
Member Author

hadley commented Oct 27, 2019

This helps a lot, but there's a major drawback: the breaks always start within the limits, so this doesn't generate any minor breaks between the minimum and first break, and the last break and the maximum.

You can see the problem with a log scale with 10 minor breaks between each major break — you should see breaks to the left of 1.

Screenshot 2019-10-27 at 9 53 21 AM

@hadley
Copy link
Member Author

hadley commented Oct 27, 2019

But maybe I can handle that by also using the limits, and doing something more sophisticated than just using a fixed number of breaks for log scales.

@paleolimbot
Copy link
Member

Just a quick note...that could partially be fixed by removing break censoring in ScaleContinuous$get_breaks(). scales::extended_breaks() often returns breaks that are outside the limits, and scales::log_breaks() I'm sure could be modified to always return breaks outside the limits. Breaks would still need to be censored before they are drawn, but this could be moved to guide_train(). There was a comment to this effect in a previous version of Scale that I removed in one of my PRs over the summer (sorry!).

@hadley
Copy link
Member Author

hadley commented Oct 27, 2019

Having briefly explored the possibility of using the range, I think your suggestion would be a better approach @paleolimbot.

@paleolimbot
Copy link
Member

I seem to remember trying this once, and the trick is getting manually specified labels to work right (manually specified labels have to be the same length as the within-limit breaks).

@thomasp85
Copy link
Member

@hadley where are we on this?

@hadley
Copy link
Member Author

hadley commented Dec 13, 2019

Lets hold off on this as it needs more changes to be fully useful.

@TyStark
Copy link

TyStark commented Aug 5, 2021

minor_breaks
Hello, I am new to contributing to open source, but I was trying to contribute a solution to this exact situation with minor_breaks on a log10 scale when I found this PR. It kept coming up at work and there seemed to be no convenient solution. The attached image is from a function I wrote that is supplied to minor_breaks that generalizes to any axis on a log10 scale. As you can see, it includes minor breaks past the last major breaks. I'm not exactly sure how to implement this, it doesn't seem like I can simply create a variable set to my function in scale-.r or scale-continuous.r, but with some help I may be able to help get this functionality into ggplot2.

@thomasp85
Copy link
Member

@hadley is it viable to get this into the next release?

@hadley
Copy link
Member Author

hadley commented May 11, 2022

@thomasp85 I have no idea

@teunbrand
Copy link
Collaborator

Breaks would still need to be censored before they are drawn, but this could be moved to guide_train().

I explored this for a bit, but there are plenty of places outside the guides where that approach gives issues, mostly in coords when dealing with the panel grid.

I think the change in the PR improves the current situation, and any 'full' solution would include these changes as well.
Therefore, I'll suggest to merge this PR under the banner of incremental improvement and deal with the out-of-bounds breaks separately once we find a satisfactory solution to that problem.

@teunbrand teunbrand modified the milestone: ggplot2 3.5.0 Nov 21, 2023
teunbrand added a commit to teunbrand/ggplot2 that referenced this pull request Dec 7, 2023
teunbrand added a commit that referenced this pull request Dec 8, 2023
* reimplement #3591

* Do not censor major breaks

* view scales censor major breaks after calculation of minor breaks

* guides censor breaks

* tests expect non-censored breaks

* add news bullet

* adjust docs

* fix typo
@teunbrand
Copy link
Collaborator

Closing this PR in favour of #5569.

@teunbrand teunbrand closed this Dec 8, 2023
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.

More flexibility for minor_breaks
5 participants