Fix scale- and coord-related regressions #3566

paleolimbot · 2019-10-11T02:20:51Z

This PR fixes two regressions I introduced in #3398 and #3426. The first affected plots where labels and breaks were specified manually (#3558):

library(ggplot2)
ggplot(mpg, aes(cty, hwy)) +
  geom_point() +
  scale_x_continuous(
    breaks = c(0, 20, 40),
    labels = c("0", "20", "40")
  )
#> Error: Breaks and labels are different lengths

This was because I censored breaks at the wrong place when I introduced ViewScales. This is fixed in this PR.

The second involves plots where there is no data and NA limits (#3560). I think NA limits should be considered "empty" when there is no data, which is how I implemented it in this PR. I also made sure that functional limits work on all discrete scales (#3448), and that NA limits aren't replaced with data limits on discrete scales (I don't even know what that would look like but probably has induced more than one spurious error). Feel free to debate how I should implement either of these.

library(ggplot2)
ggplot(data.frame(x = numeric(0)), aes(x)) + 
  geom_point() +
  scale_x_continuous(limits = c(0, NA))
#> Warning in rep(yes, length.out = len): 'x' is NULL so the result will be
#> NULL
#> Error in ans[ypos] <- rep(yes, length.out = len)[ypos]: replacement has length zero

…NA in the limits

… and continuous scales

thomasp85 · 2019-10-21T07:38:18Z

R/scale-.r

+  is_empty = function(self) {
+    has_data <- !is.null(self$range$range)
+    has_limits <- is.function(self$limits) || (!is.null(self$limits) && all(is.finite(self$limits)))
+    !has_data && !has_limits
+  },
+


Can this be added to the Scale class instead or will it break ScaleDiscrete?

I think this will cause errors in ScaleDiscrete, where NA is a perfectly valid limit (I think)

Whoops...I think that adding this implementation of is_empty() to Scale will cause errors in ScaleDiscrete.

thomasp85 · 2019-10-21T07:39:34Z

R/scale-.r

+  get_limits = function(self) {
+    if (self$is_empty()) {
+      return(c(0, 1))
+    }
+
+    if (is.null(self$limits)) {
+      self$range$range
+    } else if (is.function(self$limits)) {
+      # if limits is a function, it expects to work in data space
+      self$trans$transform(self$limits(self$trans$inverse(self$range$range)))
+    } else {
+      # NA limits for a continuous scale mean replace with the min/max of data
+      ifelse(is.na(self$limits), self$range$range, self$limits)
+    }
+  },
+


Is this just the same as Scale$get_limits()?

It's different...Scale$get_limits() doesn't do anything to NA limits, and doesn't apply the function to untransformed limits and then transform the result (because transforms only exist in continuous scales). I'm not sure what was happening with NA limits in discrete scales before.

Can you test if it breaks anything in ScaleDiscrete? I don’t like having such a small method overwriting for no reason

The reason is that passing functions as limits for discrete scales errored before (#3448), because Scale$trans is not defined. I've added tests for limit handling for continuous scales, and there are no failing tests otherwise...would adding a similar suite of tests for discrete scales be a reasonable way to resolve this?

paleolimbot · 2019-11-05T01:03:04Z

Just a ping on this in case it got buried...I'm wondering what the best way to resolve the Scale issue is. As I understand it, your concern is that discrete scales will be affected by the modifications. I added tests for ScaleContinuous$get_limits() and would be happy to do so for ScaleDiscrete$get_limits(), but I was waiting for clarification on which part of ScaleDiscrete$get_limits()'s behaviour you were worried would break. Let me know when you get the chance!

thomasp85 · 2019-11-14T09:58:29Z

I think I was just trying to be too clever about the implementation... this is prob the way to go

cpsievert · 2019-12-12T16:40:57Z

After e2e343f, it's now possible for break_positions() and get_breaks() to return NA values, is that intentional? It does raise an issue for some logic in plotly and shiny as both of them rely on these values.

library(ggplot2)

df <- data_frame(
  time = factor(c("Lunch","Dinner"), levels = c("Lunch","Dinner")),
  total_bill = c(14.89, 17.23)
)

gg <- ggplot(data = df, aes(x = time, y = total_bill, group = 1)) +
  geom_line() +
  ylim(0, max(df$total_bill))

gg <- ggplot_build(gg)

gg$layout$panel_params[[1]][["y"]]$break_positions()
#> [1] 0.04545455 0.30926502 0.57307550
#> [4] 0.83688598         NA

Also, for historical reference, the "old" way of obtaining breaks (i.e., gg$layout$panel_params[[1]][["y.major"]]) didn't return NA values either

paleolimbot · 2019-12-14T01:27:31Z

I think that it has to be that way...before this PR I'd removed the NA, which resulted in manually-specified labels/breaks failing. This reflects the value that has always been given by $get_breaks() (which sometimes contains NAs for values outside the limits). guide_axis() removes break/label combos with non-finite breaks, which I think is reasonable (but feel free to convince me otherwise!).

Note that we're considering not censoring these values at all in #3591, which would mean that some break_positions() may be outside the range [0, 1] (this will help get better minor_breaks).

thomasp85 · 2020-01-06T07:19:12Z

@cpsievert @paleolimbot what is the status on the last bit of discussion? It sounds like ggplot2 is fixed in the new approach - has plotly/shiny been updated to work with it?

paleolimbot · 2020-01-06T15:42:24Z

@cpsievert I'd be happy to PR something into plotly/shiny if you give me some guidance as to where the fix should go!

cpsievert · 2020-01-06T23:11:59Z

plotly/shiny haven't yet been updated but I'll have a closer look within the next 24 hours

As discussed in tidyverse/ggplot2#3566 (comment), it's now possible for the ggplot2 labels/positions to contains missing values, and we should be able to simply ignore them

cpsievert · 2020-01-09T20:04:05Z

Oh, forgot to follow-up --- there's a workaround for this on plotly@master now

* New version of scales no longer 'spans the gamut' of a qualitative color palette * reduce Reduce() for merging lists use do.call() instead to avoid quadratic complexity of growing lists * ensure_one(): reduce noise if attrs are NULL ignore NULL values * ggplotly: fix Xaxis anchor if the last row incomplete attach X axis to the last non-empty row in a column * stylistic changes * validate new visual baselines * Doesn't seem like we need to handle yaxis being a factor and if we do, it likely needs a different fix * update shinytest baselines * ugh maintaining shinytest tests for htmlwidgets is a real pain * update sf baselines * go back to using ggplotly() to prepare the widget for shiny rendering * the extra call to plotly_build() forces more computation then necessary and fixes plotly#1569 * Generation of non-intercept data in hline/vline should respect coord_flip(), closes plotly#1519 * update news; fix typo * Account for new changes in ggplot2's internal API, fixes plotly#1561 * Break values of positional scales have moved from to * Text labels of positional scales have moved from to * sf graticule degree labels are now quoted? * include braces * gsub * moar braces * new sf baselines * update news * upgrade to plotly.js v1.49.0 * upgrade to plotly.js v1.49.2 * upgrade to plotly.js v1.49.4 * update baseline * update shinytest baseline * make a deep copy of x.layout.width/x.layout.height for use in the resize method, closes plotly#1553 * Wrap user-supplied expression in evalq(), closes plotly#1528 * Use shiny::installExprFunction to register debug hooks * Asynchronously register plot events * Revert "Merge pull request plotly#1539 from ropensci/renderWidgetPrep" This reverts commit d416cea, reversing changes made to d11bb5a. * no need to be making deep copies * Improvements to algorithm for attaching key attribute for shiny's event data, fixes plotly#1610 * Account for pointNumbers * Do nothing if we don't recognize the case * When pointNumber is a constant, relay it as such * handle the 3 cases separately * use idx instead of i * update news * Call setInputValue only if it's defined, closes plotly#1624 * Revert "Revert "Merge pull request plotly#1539 from ropensci/renderWidgetPrep"" This reverts commit 2856731. * pass along a quoted expr that calls func() * Revert "go back to using ggplotly() to prepare the widget for shiny rendering" This reverts commit a0fa68f. * update news * verify_webgl() shouldn't warn about types that are already gl, closes plotly#1569 * fix test * Do not create structure with NULL to remove warnings * Fix management of environments in renderPlotly(), fixes plotly#1639 Both assignment and evaluation of func() should happen in the local environment * Add test to prove plotly#1299 fix * proposed change to avoid peeking at 'grid' unit internals * improve previous commit * refactor Paul's unit type commits from plotly#1646 * Some CRAN builds don't have pandoc * plot_mapbox() shouldn't force a scattermapbox trace type, closes plotly#1608 * fix R CMD check notes * r-devel apparently doesn't yet have grid::unitType() * remove outdated travis config * Validate changes visual baselines due to changes in R3.6's new RNG method * Add support for new plotly_sunburstclick event, closes plotly#1648 * validate new shinytest baselines * update news * fix silly mistake from 3cbccfc * document * test expectation should account for changes in the default color palette on r-devel * improve the aforementioned test example and add a visual test * Ignore calculated aesthetics that match specified aesthestics After tidyverse/ggplot2@10fa0014, it's possible for calculated aes to exist for all default_aes * fix broken link raster2uri and improve the description * cran hyperlinks must be https * fix roxygen warning * In contrast to attr(x, unit), the new grid::unitType() function always return a vector the same length as it's input * bump to release version; submit to CRAN * Respect sf's plot12 graticule attribute when building axis ticks, fixes plotly#1673 * Remove missing values in ticktext/tickvals As discussed in tidyverse/ggplot2#3566 (comment), it's now possible for the ggplot2 labels/positions to contains missing values, and we should be able to simply ignore them * link to comment * remove outdated/bad test expectation...the visual baseline covers it * Don't assume sf geometry is always accessible via [['geometry']], fixes plotly#1659 * validate baselines * update news * update to plotly.js 1.52.2 * new baselines * Add image and treemap visual tests. Also, ensure treemap's stroke reflects the paper's bgcolor * Add Object.setPrototypeOf polyfill for shinytest and update shinytest json baseline * Add add_image() for easier rendering of image traces via raster objects * Bump version, update news, document, add unit test for add_image() * roxygen escapes % now * Only call locale_dependency() for locales not included with standard bundle, closes plotly#1686 * CRAN submission * plot_mapbox() should default to scattermapbox unless z is present, fixes plotly#1707 * add visual test for plotly#1674 * update poor test expectations that assume stringsAsFactors defaults to TRUE * bump version; update news * update visual tests * remove rnaturalearth dependency in tests Co-authored-by: Carson Sievert <[email protected]> Co-authored-by: Alexey Stukalov <[email protected]> Co-authored-by: Wim <[email protected]> Co-authored-by: Paul Murrell <[email protected]>

* Start v4.9.4 release candidate (plotly#1959) * Add crosstalk mapping to computed_mapping (not mapping) when present (plotly#1964) * Follow up to plotly#1952: add crosstalk mapping to computed_mapping (not mapping) when present * use dev version; update news * v4.9.4.1 release candidate (plotly#1965) * v4.9.4.1 release candidate * Reduce some of the warning noise * Reduce some of the warning noise * use dev version * Upgrade to plotly.js 2.0 (plotly#1967) * wip upgrade to plotly.js 2.0 * patch plotly.js source to support phantomjs/shinytest reverts part of plotly/plotly.js#5380 * bump mathjax version * use npm instead of yarn * make geom_bar() default to textposition='none'; approve new vdiffr 0.4 + plotly.js 2.0 baselines * Migrate to vdiffr 1.0 * Use layout.legend.title over layout.annotations to when converting guides for discrete scales * wip migrate to gha * try npm install * setup node * use recommended node version * try mac instead * install phantomjs; various cleanup * intentionally break a visual test * fix * use thematic's approach to testing * bump version * regenerate snapshots * map secrets to env variables * accept new gha baselines * whoops * hmm * approve shinytest baseline * intentionally break a visual test * always upload * revert change; new baselines * Remove the vdiffr dependency and use testthat snaphots directly * More obvious env var naming * more gha details * always upload * new snaps * intentionally break a visual test (again) * Revert "intentionally break a visual test (again)" This reverts commit a4e306c. * Update subplots.R (plotly#1974) To get rid of "Error is get_domains(length(plots), nrows, margin, widths = widths, heights = heights): The sum of the widths and heights arguments must be less than 1. * Better positioning of facet axis annotations (plotly#1975) * Add kaleido() for static image exporting (plotly#1971) * Close plotly#1970: revert plotly.js 2.0 change in default modebar buttons * Add save_image(), a convenience wrapper around kaleido()$transform() (plotly#1981) * Upgrade to plotly.js v2.2.1 (plotly#1982) * Use Plotly.react() to redraw in Shiny if layout.transition is populated (plotly#2001) * Use Plotly.react() to redraw in Shiny if layout.transition is populated * update news * Update to plotly.js v2.5.1 (plotly#2002) * update to plotly.js v2.5.1 * new visual baselines * Closes plotly#2031. ggplot internaly convert `color` to `colour` * Update README.md * update baselines * update URLs * add_area() now uses barpolar instead of area (plotly#2041) * Closes plotly#1872. Implemented to_basic for `geom_function` * 4.10.0 release * Added unit tests. And ran tests using * Conformed with carson's review * Reverted the deletion of _snaps * Added test to check whether the fix works or not. * Added test to check whether the fix works or not. * use dev versioning * Fix "partial argument match" warnings (plotly#1977) * No partial-argument-match warning in ggplotly * Update NEWS.md * Solved the LaTeX2exp error. Closes plotly#2027 (plotly#2030) Co-authored-by: Carson Sievert <[email protected]> * Revert "Solved the LaTeX2exp error. Closes plotly#2027 (plotly#2030)" This reverts commit aeaecd6. * Add ggalluvial support (plotly#2061) Co-authored-by: Carson Sievert <[email protected]> Co-authored-by: Abdessabour Moutik <[email protected]> * Fix ordering of lines in stat_ecdf() (plotly#2065) * Handle geom_tile() with no fill aesthetic (plotly#2063) * More careful joining of group columns (plotly#2064) * Avoid with() to better account for missing tickvals/ticktext (plotly#2062) * Test discrete-ness of fill after statistics have been calculated (plotly#2066) * Announce snapshot files when visual testing isn't enabled so they won't get deleted * Respect guide(aes = 'none') when constructing legend entries (plotly#2067) * Add 'plotly_selecting' to acceptable 'on' events (plotly#1280) * Use faster versions of system.file()/packageVersion()/is_installed() (plotly#2072) * Bump plotly.js to 2.11.1 (plotly#2096) * approve new visual baselines * Use rlang::local_options() in test * new shinytest baselines; avoid rlang altogether * plotly.js fix bug when crosstalk filter keys are null (plotly#2087) * transition `tidyr::gather_` -> `tidyr::pivot_longer` (plotly#2125) * transition `tidyr::gather_` -> `tidyr::pivot_longer` * add `NEWS` entry * Replace `is.na` with `rlang::is_na` in `map_color()` (plotly#2131) * roxygenize * Support renderValue within iframe * Add yaml for auto-P.R./fork updating; RS-2202 * [pull] master from ropensci:master (#2) * New version of scales no longer 'spans the gamut' of a qualitative color palette * reduce Reduce() for merging lists use do.call() instead to avoid quadratic complexity of growing lists * ensure_one(): reduce noise if attrs are NULL ignore NULL values * ggplotly: fix Xaxis anchor if the last row incomplete attach X axis to the last non-empty row in a column * stylistic changes * validate new visual baselines * Doesn't seem like we need to handle yaxis being a factor and if we do, it likely needs a different fix * update shinytest baselines * ugh maintaining shinytest tests for htmlwidgets is a real pain * update sf baselines * go back to using ggplotly() to prepare the widget for shiny rendering * the extra call to plotly_build() forces more computation then necessary and fixes plotly#1569 * Generation of non-intercept data in hline/vline should respect coord_flip(), closes plotly#1519 * update news; fix typo * Account for new changes in ggplot2's internal API, fixes plotly#1561 * Break values of positional scales have moved from to * Text labels of positional scales have moved from to * sf graticule degree labels are now quoted? * include braces * gsub * moar braces * new sf baselines * update news * upgrade to plotly.js v1.49.0 * upgrade to plotly.js v1.49.2 * upgrade to plotly.js v1.49.4 * update baseline * update shinytest baseline * make a deep copy of x.layout.width/x.layout.height for use in the resize method, closes plotly#1553 * Wrap user-supplied expression in evalq(), closes plotly#1528 * Use shiny::installExprFunction to register debug hooks * Asynchronously register plot events * Revert "Merge pull request plotly#1539 from ropensci/renderWidgetPrep" This reverts commit d416cea, reversing changes made to d11bb5a. * no need to be making deep copies * Improvements to algorithm for attaching key attribute for shiny's event data, fixes plotly#1610 * Account for pointNumbers * Do nothing if we don't recognize the case * When pointNumber is a constant, relay it as such * handle the 3 cases separately * use idx instead of i * update news * Call setInputValue only if it's defined, closes plotly#1624 * Revert "Revert "Merge pull request plotly#1539 from ropensci/renderWidgetPrep"" This reverts commit 2856731. * pass along a quoted expr that calls func() * Revert "go back to using ggplotly() to prepare the widget for shiny rendering" This reverts commit a0fa68f. * update news * verify_webgl() shouldn't warn about types that are already gl, closes plotly#1569 * fix test * Do not create structure with NULL to remove warnings * Fix management of environments in renderPlotly(), fixes plotly#1639 Both assignment and evaluation of func() should happen in the local environment * Add test to prove plotly#1299 fix * proposed change to avoid peeking at 'grid' unit internals * improve previous commit * refactor Paul's unit type commits from plotly#1646 * Some CRAN builds don't have pandoc * plot_mapbox() shouldn't force a scattermapbox trace type, closes plotly#1608 * fix R CMD check notes * r-devel apparently doesn't yet have grid::unitType() * remove outdated travis config * Validate changes visual baselines due to changes in R3.6's new RNG method * Add support for new plotly_sunburstclick event, closes plotly#1648 * validate new shinytest baselines * update news * fix silly mistake from 3cbccfc * document * test expectation should account for changes in the default color palette on r-devel * improve the aforementioned test example and add a visual test * Ignore calculated aesthetics that match specified aesthestics After tidyverse/ggplot2@10fa0014, it's possible for calculated aes to exist for all default_aes * fix broken link raster2uri and improve the description * cran hyperlinks must be https * fix roxygen warning * In contrast to attr(x, unit), the new grid::unitType() function always return a vector the same length as it's input * bump to release version; submit to CRAN * Respect sf's plot12 graticule attribute when building axis ticks, fixes plotly#1673 * Remove missing values in ticktext/tickvals As discussed in tidyverse/ggplot2#3566 (comment), it's now possible for the ggplot2 labels/positions to contains missing values, and we should be able to simply ignore them * link to comment * remove outdated/bad test expectation...the visual baseline covers it * Don't assume sf geometry is always accessible via [['geometry']], fixes plotly#1659 * validate baselines * update news * update to plotly.js 1.52.2 * new baselines * Add image and treemap visual tests. Also, ensure treemap's stroke reflects the paper's bgcolor * Add Object.setPrototypeOf polyfill for shinytest and update shinytest json baseline * Add add_image() for easier rendering of image traces via raster objects * Bump version, update news, document, add unit test for add_image() * roxygen escapes % now * Only call locale_dependency() for locales not included with standard bundle, closes plotly#1686 * CRAN submission * plot_mapbox() should default to scattermapbox unless z is present, fixes plotly#1707 * add visual test for plotly#1674 * update poor test expectations that assume stringsAsFactors defaults to TRUE * bump version; update news * update visual tests * remove rnaturalearth dependency in tests Co-authored-by: Carson Sievert <[email protected]> Co-authored-by: Alexey Stukalov <[email protected]> Co-authored-by: Wim <[email protected]> Co-authored-by: Paul Murrell <[email protected]> * Add back missing layout attribute * Pass element instead of ID * VIS-992: add test, use rhtmlBuildUtils (#15) * Initial commit * Remove prepush * Final line for plotly.yml * Re-add lib * Refactor * Create test * Specify rhtmlBuildUtils version * Remove preinstall script * Delete package-lock.json * Update documentation * Update documentation and remove library * Restore files * Restore file * Update build files * VIS-933: add rhtmlwidget-status attribute (#16) * Test callback * Update compiled files * Add rhtmlwidget-status attribute * wording change * VIS-1064: fix PPT exporting issue (#17) * Candidate fix * Bump version * Update widgetdefinition.js and build Co-authored-by: Carson Sievert <[email protected]> Co-authored-by: Abhilash Lakshman <[email protected]> Co-authored-by: Abdessabour Moutik <[email protected]> Co-authored-by: Jack Parmer <[email protected]> Co-authored-by: bersbersbers <[email protected]> Co-authored-by: casperhart <[email protected]> Co-authored-by: Simon P. Couch <[email protected]> Co-authored-by: Paul Hoffman <[email protected]> Co-authored-by: Jeffrey Shen <[email protected]> Co-authored-by: flipDevTools <[email protected]> Co-authored-by: pull[bot] <39814207+pull[bot]@users.noreply.github.com> Co-authored-by: Alexey Stukalov <[email protected]> Co-authored-by: Wim <[email protected]> Co-authored-by: Paul Murrell <[email protected]> Co-authored-by: Justin Yap <[email protected]>

paleolimbot added 3 commits October 10, 2019 21:19

fix failure with censored breaks when labels were specified manually

e2e343f

ensure that ScaleContinuous$is_empty() returns TRUE when there is an …

0a98b65

…NA in the limits

ensure that functional and NA limits are handled properly on discrete…

b8768be

… and continuous scales

paleolimbot requested a review from thomasp85 October 11, 2019 12:46

thomasp85 reviewed Oct 21, 2019

View reviewed changes

thomasp85 approved these changes Nov 14, 2019

View reviewed changes

paleolimbot merged commit 86cc4d2 into tidyverse:master Nov 14, 2019

paleolimbot deleted the issue-3558-break-censoring branch November 14, 2019 15:28

paleolimbot mentioned this pull request Dec 20, 2019

Release ggplot2 3.3.0 #3674

Closed

18 tasks

cpsievert mentioned this pull request Jan 6, 2020

Remove missing values in ticktext/tickvals plotly/plotly.R#1679

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scale- and coord-related regressions #3566

Fix scale- and coord-related regressions #3566

paleolimbot commented Oct 11, 2019

thomasp85 Oct 21, 2019

paleolimbot Oct 21, 2019

paleolimbot Oct 21, 2019

thomasp85 Oct 21, 2019

paleolimbot Oct 21, 2019

thomasp85 Oct 21, 2019

paleolimbot Oct 21, 2019

paleolimbot commented Nov 5, 2019

thomasp85 commented Nov 14, 2019

cpsievert commented Dec 12, 2019 •

edited

Loading

paleolimbot commented Dec 14, 2019

thomasp85 commented Jan 6, 2020

paleolimbot commented Jan 6, 2020

cpsievert commented Jan 6, 2020

cpsievert commented Jan 9, 2020

Fix scale- and coord-related regressions #3566

Fix scale- and coord-related regressions #3566

Conversation

paleolimbot commented Oct 11, 2019

thomasp85 Oct 21, 2019

Choose a reason for hiding this comment

paleolimbot Oct 21, 2019

Choose a reason for hiding this comment

paleolimbot Oct 21, 2019

Choose a reason for hiding this comment

thomasp85 Oct 21, 2019

Choose a reason for hiding this comment

paleolimbot Oct 21, 2019

Choose a reason for hiding this comment

thomasp85 Oct 21, 2019

Choose a reason for hiding this comment

paleolimbot Oct 21, 2019

Choose a reason for hiding this comment

paleolimbot commented Nov 5, 2019

thomasp85 commented Nov 14, 2019

cpsievert commented Dec 12, 2019 • edited Loading

paleolimbot commented Dec 14, 2019

thomasp85 commented Jan 6, 2020

paleolimbot commented Jan 6, 2020

cpsievert commented Jan 6, 2020

cpsievert commented Jan 9, 2020

cpsievert commented Dec 12, 2019 •

edited

Loading