Skip to content

Conversation

thomasp85
Copy link
Member

I just came across a spectacular failure of the new bin scale in combination with the new axis implementation. This PR adresses the two issues: Make sure guide is set to waiver() instead of "none". And make sure that breaks are not mapped by the scale unless it is a discrete scale. The old approach worked before because mapping with a continuous position scale was a no-op, but this is not true for the bin scale and it would throw the axis completely off.

@thomasp85 thomasp85 added this to the ggplot2 3.3.0 milestone Jan 7, 2020
@thomasp85 thomasp85 requested a review from paleolimbot January 7, 2020 10:32
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

It seems like the root cause of this is that breaks have a slightly different meaning in a binned scale. A deeper fix would be to create a guide_axis() subclass that correctly positions the ticks between bins in the guide_train() method (the default method could use interval notation and position the tick in the middle of the bin as usual). This would also make for a better guide_legend() when using ScaleBinned (currently the labels are integers if you do this). I don't think it's unreasonable for position bin scales to use their own axis guide, given that they do this for all the other guides already.

That said, I'm totally fine with this fix, as continuous position scales will probably never map their breaks.

@thomasp85
Copy link
Member Author

I don't completely agree with your analysis. Breaks are the same between continuous and binned scales, i.e. a numeric along a continuous interval. It is the discrete scale that is the odd one as it converts the non-numeric values into the same type as that of continuous and binned. This is also the only reason why a mapping in needed and it makes sense to do it based on a discrete-check.

As bins don't necessarily have the same size I don't think it makes sense to have the default at the center - it would not be possible to deduce the correct border position solemnly on the centers

@thomasp85 thomasp85 merged commit 94b5a16 into tidyverse:master Jan 7, 2020
@paleolimbot
Copy link
Member

You're probably right...I don't have any experience with the binned scale, but it looks very cool!

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.

2 participants