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

add explicit parsing option to labels in coord_sf() #2881

Merged

Conversation

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Sep 4, 2018

Thanks to @slowkow's work on label parsing, I can now see how to make parsing of labels configurable in coord_sf(). With this PR, labels are only autoparsed if they were actually generated by sf::st_graticule(). Otherwise, they only get parsed if explicitly requested in coord_sf().

@clauswilke
Copy link
Member Author

@clauswilke clauswilke commented Sep 5, 2018

Examples:

library(sf)
#> Linking to GEOS 3.6.1, GDAL 2.1.3, proj.4 4.9.3
library(ggplot2) 

nc <- st_read(system.file("gpkg/nc.gpkg", package="sf"))
#> Reading layer `nc.gpkg' from data source `/Library/Frameworks/R.framework/Versions/3.5/Resources/library/sf/gpkg/nc.gpkg' using driver `GPKG'
#> Simple feature collection with 100 features and 14 fields
#> geometry type:  MULTIPOLYGON
#> dimension:      XY
#> bbox:           xmin: -84.32385 ymin: 33.88199 xmax: -75.45698 ymax: 36.58965
#> epsg (SRID):    4267
#> proj4string:    +proj=longlat +datum=NAD27 +no_defs

# default labels are parsed when needed
p_nc <- ggplot() + 
  geom_sf(aes(fill = AREA), data=nc)
p_nc

p_polygon <- ggplot(sf::st_polygon(list(matrix(1e6*c(1, 2, 3, 1, 1, 3, 2, 1), ncol = 2)))) + 
  geom_sf(size = 3, fill = 'pink')
p_polygon

  
# explicitly supplied labels are not parsed by default
p_nc +
  scale_y_continuous(
    breaks = c(34, 35, 36),
    labels = c("34 * degree * N", "35 * degree * N", "36 * degree * N")
  )

# but parsing can be turned on
p_nc + 
  coord_sf(parse_N_labels = TRUE) +
  scale_y_continuous(
    breaks = c(34, 35, 36),
    labels = c("34 * degree * N", "35 * degree * N", "36 * degree * N")
  )

# parsing doesn't need to be turned on if labels are provided in parsed format

# labeling function
pow10_format <- function(x) scales::math_format()(signif(log10(x), 3))
p_polygon + 
  scale_x_continuous(labels = pow10_format) +
  scale_y_continuous(labels = pow10_format)

# explicit
p_polygon + 
  scale_x_continuous(
    breaks = c(1e6, 2e6, 3e6),
    labels = parse(text = c("10^6", "2 %*% 10^6", "3 %*% 10^6"))
  )

Created on 2018-09-04 by the reprex package (v0.2.0).

@clauswilke clauswilke requested a review from hadley Sep 5, 2018
R/sf.R Outdated
@@ -383,6 +383,10 @@ scale_type.sfc <- function(x) "identity"
#' @usage NULL
#' @format NULL
CoordSf <- ggproto("CoordSf", CoordCartesian,
# parameters that control the parsing of labels plotted in the
# N or E directions
parse_N_labels = FALSE,

This comment has been minimized.

@hadley

hadley Sep 12, 2018
Member

Can we just have the user pass expressions as labels rather than making this an option?

@hadley
hadley approved these changes Sep 12, 2018
Copy link
Member

@hadley hadley left a comment

Implementation seems fine, but I'd like to make sure we don't introduce arguments that we don't have to walk back later. OTOH, I don't want this to block the release, so if I stop responding feel free to go ahead and merge.

@clauswilke
Copy link
Member Author

@clauswilke clauswilke commented Sep 12, 2018

We can remove the parameters. My first iteration of the code couldn't handle expressions, so the parameters were needed, but then I realized I could support expressions directly. I'll remove these parameters. It would be easy to add them back in later if we feel we need that option.

@clauswilke clauswilke mentioned this pull request Sep 12, 2018
25 tasks done
@clauswilke clauswilke force-pushed the clauswilke:coord_sf-customize-label-parsing branch from c8302dc to 6527b4f Sep 13, 2018
@clauswilke
Copy link
Member Author

@clauswilke clauswilke commented Sep 13, 2018

@hadley If you have a moment, could you look this over one more time? The two main changes I made are:

  1. I removed the parameters in coord_sf(). They are not needed.
  2. I treat factors like character vectors but treat all other non-character objects as things that should be parsed before drawing. As a result, the following variable types are all handled correctly: character vectors, factors, expression objects, lists containing expression objects. But some other variable type could cause unexpected results. Is this a problem? Is there some type of object that users would expect to behave like character vectors but that is neither a character vector nor a factor? (Note: functions are also handled correctly.)
@hadley
hadley approved these changes Sep 13, 2018
Copy link
Member

@hadley hadley left a comment

I think even handling factors is going above and beyond what's necessary. LGTM

R/sf.R Outdated
if (is.character(x_labels)) {
# all labels need to be temporarily stored as character vectors,
# but expressions need to be parsed afterwards
if (is.character(x_labels) || is.factor(x_labels)) {

This comment has been minimized.

@hadley

hadley Sep 13, 2018
Member

Could this just be needs_parsing[graticule$type == "E"] <- is.character(x_labels) || is.factor(x_labels) ?

This comment has been minimized.

@clauswilke

clauswilke Sep 13, 2018
Author Member

It needs a not in front, but otherwise yes, thanks!

@clauswilke clauswilke merged commit 1e01e68 into tidyverse:master Sep 13, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@clauswilke clauswilke deleted the clauswilke:coord_sf-customize-label-parsing branch Sep 13, 2018
@lock
Copy link

@lock lock bot commented Mar 12, 2019

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/

@lock lock bot locked and limited conversation to collaborators Mar 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants