Better handling of numeric levels in scale_discrete #1708
Conversation
|
Can you please add two simple unit tests (one for each issue) using |
|
Are these unit tests fine? |
| @@ -16,3 +16,19 @@ test_that("discrete ranges also encompas continuous values", { | |||
| expect_equal(x_range(base + geom_point(aes(x1)) + geom_point(aes(x2))), c(0, 4)) | |||
| }) | |||
|
|
|||
| test_that("discrete scale shrinks to range when setting limits", { | |||
| p <- ggplot(subset(diamonds, carat > 1), aes(cut, clarity)) + | |||
|
I think we definitely need to handle this case – i.e. when a scale has been trained with continuous data, but not discrete data, we should not generate any breaks by default. I don't think that will break any existing behaviour. |
|
As in "any breaks at all"? wasn't this the behaviour that get marked as a bug in #1589? |
|
Ah, in that case #1589 is not a bug, but a deliberate design decision. |
|
Oh - that's kind of an important observation :-) |
|
Lets see if I can untangle the fix for #1638 from this then |
|
And maybe we should have a warning for #1589 — i.e. something like "Discrete scale used, but only given continuous data."? |
|
yeah, I think there is definitely a chance of confusion with this design |
|
Yes, but I'm not sure why you're applying a discrete scale to continuous data! |
|
No idea either, but with 27k downloads/month it is sure that it will get attempted at a regular basis |
| @@ -77,6 +77,9 @@ | |||
|
|
|||
| * A warning is now issued when a scale transformation introduces infinite | |||
| values in a scale (#1696) | |||
|
|
|||
| * Fixed bug where space for dropped levels in scale_discrete would be preserved | |||
| (#1638) | |||
|
Apart from that small tweak to NEWS, LGTM |
|
I've added a warning mechanism for #1589 as we discussed - please review that. It is caching whether it has already warned inside the scale object itself which is not ideal, but it was the only way I could avoid replicated warnings |
| @@ -132,6 +134,14 @@ ScaleDiscretePosition <- ggproto("ScaleDiscretePosition", ScaleDiscrete, | |||
| } | |||
| }, | |||
|
|
|||
| get_breaks = function(self, limits = self$get_limits()) { | |||
| if (self$warn && length(self$get_limits()) == 0) { | |||
There was a problem hiding this comment.
If you use warn, I think you'd need to reset it in the clone() method below.
What if in Scale$train_df() we called train_init() and train_final() methods? (or begin/end, setup/teardown, start/finish) Then you could call this in train_finalise(). (I'm not sure what you'd use train_init() for, but it makes sense for symmetry)
There was a problem hiding this comment.
Having a function that is called between training and mapping seems most important — we could also use it to cache the palette for discrete scales. Given that we need for two scales, it suggests it might be useful in other places. Let's call it train_complete().
There was a problem hiding this comment.
Should we ignore train_start() then and only go for a complete hook?
There was a problem hiding this comment.
That's fine but there's something nice about the summery of having both
There was a problem hiding this comment.
Hmm... there are some problems with our clever plan. The obvious place to put these would be at the beginning and end of train_df, but this method is only called on non-positional scales. For positional scales train is called directly - further, it is called twice and if there are multiple scales due to facetting it is called for each of them... Not saying we shouldn't add the methods, but it doesn't solve the problem we are trying to solve...
|
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/ |
Fixes #1638 and #1589