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

Discrete x/y scales reserve space for unused limits #1638

Closed
wch opened this Issue May 29, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@wch
Member

wch commented May 29, 2016

For example, from the docs

d <- ggplot(subset(diamonds, carat > 1), aes(cut, clarity)) +
      geom_jitter()
d + scale_x_discrete(limits=c("Fair","Ideal"))

In the 0.9.3.1 docs, it looks like this:

@wch

This comment has been minimized.

Member

wch commented May 29, 2016

I've bisected it to 89893c7.

@hadley

This comment has been minimized.

Member

hadley commented Jul 28, 2016

Probably related to #1589

@hadley hadley added bug ready labels Jul 29, 2016

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 12, 2016

This is two unrelated problems. This one is easily fixed whereas #1589 is a bit less straightforward.

@hadley can you by chance remember what you tried to achieve in 89893c7? The message is that it improves the defaults, but since it is quite broken it is not obvious what was wrong with the old.

@thomasp85 thomasp85 self-assigned this Aug 12, 2016

@hadley

This comment has been minimized.

Member

hadley commented Aug 12, 2016

The goal was to fix #1542 - ie when you accidentally use a discrete scale with only continuous data. (You have to be able to use a mix of cont and disc so that jittering and similar work)

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 12, 2016

I somehow missed that reference - sorry 'bout that

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 12, 2016

It seems your change in get_limits didn't anticipate that the function was used in other places than dimensions.

Was the change supposed to do avoid drawing all breaks when numeric ranges were converted to discrete?

@hadley

This comment has been minimized.

Member

hadley commented Aug 12, 2016

Your guess is as good as mine. (But I don't think it was supposed to)

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 12, 2016

Also is it intentional that continuous data on a discrete scale does not receives any expansion?

In general can you explain the calls to expand_range? It seems that the continuous range only receives additive expansion and discrete ranges only receives multiplicative expansion - shouldn't both receive both?

@hadley

This comment has been minimized.

Member

hadley commented Aug 12, 2016

I'm not sure I can - I've done some exploration recently and I think multiplicative looks better for both. I don't remember my original reasoning

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 12, 2016

Makes sense - additive is very dependent on the number of levels. But no matter what looks best, shouldn't it honor what is passed in through the expand argument instead of just silently ignoring some of the settings?

@hadley

This comment has been minimized.

Member

hadley commented Aug 12, 2016

The goal was always to get an aesthetic pleasing amount of space between the data and the axes. In an ideal world, that would be the same for both x and y axes, but there's no way to do that.

You're right that we should respect both options - I'm not sure why I made that change.

For this problem, the fix is probably straightforward - what we need to make sure to capture is a few different motivating examples of mixing continuous and discrete values on a discrete axis.

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 12, 2016

I'll cook up a PR with a potential fix and let you stress test it as you seems to have a better idea about what is the intended functionality... (coding in the blind FTW :-)

@hadley hadley added in progress and removed ready labels Aug 12, 2016

thomasp85 added a commit that referenced this issue Sep 16, 2016

Drop space for unused limits in scale_discrete
Fixes #1638 

* Use get_limit in call to dimension

@hadley hadley removed the in progress label Sep 16, 2016

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.