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

Adding scale_*_discrete() to date variable can crash R #1542

Closed
cpsievert opened this issue Feb 10, 2016 · 5 comments
Closed

Adding scale_*_discrete() to date variable can crash R #1542

cpsievert opened this issue Feb 10, 2016 · 5 comments

Comments

@cpsievert
Copy link
Contributor

@cpsievert cpsievert commented Feb 10, 2016

With scales 0.3, this code will error:

x <- c(Sys.Date(), Sys.Date() - 1e4)
y <- c(3, 4)
d <- data.frame(x, y)
library(ggplot2)
ggplot(d, aes(x, y)) + geom_line() + scale_x_discrete()
#> Error: Discrete value supplied to continuous scale

Running this code using the latest dev version of scales doesn't result in error, and ggplot tries to draw a plot with 1e4 ticks. I know this is a cornercase, but I would prefer an error since it can easily wreak havoc on an R session, especially with date-times.

@cpsievert
Copy link
Contributor Author

@cpsievert cpsievert commented Feb 10, 2016

Here is the relevant change -- r-lib/scales@cd0395d

@hadley
Copy link
Member

@hadley hadley commented Feb 10, 2016

Are you sure that's the change? It seems weird that a check in continuous scales would break discrete scales

@cpsievert
Copy link
Contributor Author

@cpsievert cpsievert commented Feb 12, 2016

Yes, it appears both scale_x_discrete()/scale_y_discrete() use a continuous range. I'm not sure whether or not that is intentional, but it does seem odd:

> scale_x_discrete
function (..., expand = waiver()) 
{
    sc <- discrete_scale(c("x", "xmin", "xmax", "xend"), "position_d", 
        identity, ..., expand = expand, guide = "none")
    sc$super <- ScaleDiscretePosition
    class(sc) <- class(ScaleDiscretePosition)
    sc$range_c <- continuous_range()
    sc
}

Just to demonstrate that the change I refer to above is relevant, you can install r-lib/scales#70 to see that the example now throws an error.

@hadley
Copy link
Member

@hadley hadley commented Feb 12, 2016

Ah, that makes sense. But I'd rather fix it by handing the poor performance with many levels

@hadley
Copy link
Member

@hadley hadley commented Feb 17, 2016

Hmmm, something is seriously messed up with code that handles the combination of discrete and continuous ranges. I need to think through exactly how this should behave - the reason why discrete scale also have a continuous component is so that this sort of plot can work:

ggplot(mtcars, aes(factor(cyl), mpg)) + geom_jitter()

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants