Fix warning in geom_violin with draw_quantiles #4654
Conversation
|
Thanks - we are in the process of updating our CI to the master->main branch change - I'll go through this once tests are running for PR's again |
|
Hi @bersbersbers - can I get you to merge in the main branch to kick off CI? |
tests/testthat/test-geom-violin.R
Outdated
| @@ -90,6 +98,10 @@ test_that("geom_violin draws correctly", { | |||
| expect_doppelganger("quantiles", | |||
| ggplot(dat, aes(x=x, y=y)) + geom_violin(draw_quantiles=c(0.25,0.5,0.75)) | |||
| ) | |||
| dat_outlier <- data.frame(x = 1, y = c(0, 0.25, 0.5, 0.75, 5)) | |||
There was a problem hiding this comment.
Why do we need a visual test here?
There was a problem hiding this comment.
Good question - maybe we don't. The change in
https://github.com/tidyverse/ggplot2/pull/4654/files/09b4df9ed1b61d1071c00b1d5a886f72d5a4e57d#diff-6ac793ea0483ba5c6cd55aa4b47e2a9fca57dc1d867fb0ee8851058dfae407d5R200
does not affect any of the existing visual tests, so I thought it might make sense to add a visual test that is affected by the fix and, in fact, make sure the (originally invisible) quantile line stays invisible.
There was a problem hiding this comment.
Visual tests are expensive to maintain, so they're best reserved for cases where they're absolutely necessary. So if there's any way you can test it otherwise, it'd be better to do so 😄
There was a problem hiding this comment.
I replaced the visual test by one based on the grob. I haven't found good documentation on how to best traverse this tree, though, but it's a start.
There was a problem hiding this comment.
Can you please also add a bullet to the top of NEWS.md? It should briefly describe the change and end with (@yourname, #issuenumber).
tests/testthat/test-geom-violin.R
Outdated
| p <- ggplot(df, aes(x = x, y = y)) + | ||
| geom_violin(draw_quantiles = c(0.5), bw = 0.1) | ||
|
|
||
| grob <- layer_grob(p) |
There was a problem hiding this comment.
Instead of testing the grobs, could you just test create_quantile_segment_frame() directly?
There was a problem hiding this comment.
Good idea, done. In this process, I removed the test for the non-zero middle because that is covered by "create_quantile_segment_frame functions for 3 quantiles".
|
Perfect, thanks! |
* master: (320 commits) Orientation-aware key glyphs (tidyverse#4757) Fix warning in geom_violin with draw_quantiles (tidyverse#4654) Fix misalignment in geom_dotplot when stackratio != 1 and stackdir != "up" (tidyverse#4734) Replace coord_equal in scale-identity.r (tidyverse#4759) Unified message format in `geom_smooth()` (tidyverse#4634) Add parentheses to mentions of binned_scale in scale-alpha and scale-viridis documentation (tidyverse#4735) Update geom-boxplot.r (tidyverse#4744) Remove unneeded backslash from diamonds docs (tidyverse#4711) Re-document Warn on unsupported geoms in annotate() (tidyverse#4721) Re-document Add Trump to presidential terms dataset (tidyverse#4702) Corrects bins / binwidth override documentation (tidyverse#4720) Update infrastructure to new best practices (tidyverse#4748) remove stringsAsFactor for the mapped_discrete as.data.frame method (tidyverse#4750) Workflow updates (tidyverse#4747) Ensure output is numeric even if ifelse clause is NA (tidyverse#4692) Add non_missing_aes to geom_tile (tidyverse#4683) Pass on binwidth and height to geom (tidyverse#4671) Check for range == NULL - happens if no data has been added to plot (tidyverse#4682) ...
Fix "collapsing to unique 'x' values" warnings. Closes #4455
A test will follow in a later commit.