Skip to content

coord_sf() scale function breaks #5442

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

Merged
merged 11 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# ggplot2 (development version)

* Position scales combined with `coord_sf()` can now use functions in the
`breaks` argument. In addition, `n.breaks` works as intended and
`breaks = NULL` removes grid lines and axes (@teunbrand, #4622).
* (Internal) Applying defaults in `geom_sf()` has moved from the internal
`sf_grob()` to `GeomSf$use_defaults()` (@teunbrand).
* `facet_wrap()` has new options for the `dir` argument to more precisely
Expand Down
60 changes: 58 additions & 2 deletions R/coord-sf.R
Original file line number Diff line number Diff line change
Expand Up @@ -222,16 +222,25 @@ CoordSf <- ggproto("CoordSf", CoordCartesian,
x_range[2], y_range[2]
)

breaks <- sf_breaks(scale_x, scale_y, bbox, params$crs)

# Generate graticule and rescale to plot coords
graticule <- sf::st_graticule(
bbox,
crs = params$crs,
lat = scale_y$breaks %|W|% NULL,
lon = scale_x$breaks %|W|% NULL,
lat = breaks$y %|W|% NULL,
lon = breaks$x %|W|% NULL,
datum = self$datum,
ndiscr = self$ndiscr
)

if (is.null(breaks$x)) {
graticule <- vec_slice(graticule, graticule$type != "E")
}
if (is.null(breaks$y)) {
graticule <- vec_slice(graticule, graticule$type != "N")
}

# override graticule labels provided by sf::st_graticule() if necessary
graticule <- self$fixup_graticule_labels(graticule, scale_x, scale_y, params)

Expand Down Expand Up @@ -580,6 +589,53 @@ parse_axes_labeling <- function(x) {
list(top = labs[1], right = labs[2], bottom = labs[3], left = labs[4])
}

# This function does two things differently from standard breaks:
# 1. It does not resolve `waiver()`, unless `n.breaks` is given. In the case
# that breaks are `waiver()`, we use the default graticule breaks.
# 2. It discards non-finite breaks because they are invalid input to the
# graticule. This may cause atomic `labels` to be out-of-sync.
sf_breaks <- function(scale_x, scale_y, bbox, crs) {

has_x <- !is.null(scale_x$breaks) || !is.null(scale_x$n.breaks)
has_y <- !is.null(scale_y$breaks) || !is.null(scale_y$n.breaks)

x_breaks <- if (has_x) waiver() else NULL
y_breaks <- if (has_y) waiver() else NULL


if (has_x || has_y) {
if (!is.null(crs)) {
# Atomic breaks input are assumed to be in long/lat coordinates.
# To preserve that assumption for function breaks, the bounding box
# needs to be translated to long/lat coordinates.
if (!is_named(bbox)) {
names(bbox) <- c("xmin", "ymin", "xmax", "ymax")
}
# Convert bounding box to long/lat coordinates
bbox <- sf::st_as_sfc(sf::st_bbox(bbox, crs = crs))
bbox <- sf::st_bbox(sf::st_transform(bbox, 4326))
bbox <- as.numeric(bbox)

# If any bbox is NA the transformation has probably failed.
# (.e.g from IGH to long/lat). In this case, just provide full long/lat.
bbox[is.na(bbox)] <- c(-180, -90, 180, 90)[is.na(bbox)]
}
Comment on lines +607 to +622
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like some more sf-experienced eyes on this bit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clauswilke you are perhaps the one best fitting the "sf-experienced" moniker :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked carefully yet and I'm not entirely sure I even understand what this code is meant to do, but one thing I notice right away is hard-coded CRS values (line 617) and limits (line 622). Should this respect the choice of default_crs in coord_sf()? Also, I'm worried about just transforming a bounding box. This can give all sorts of unexpected results for weird coordinate systems. That's why I added different limits methods implemented in calc_limits_bbox(). Should that be used here, and the choice of the limit method respected (see argument lims_method in coord_sf())? Finally, I added the function sf_transform_xy() to transform coordinates will imposing various checks. Is that useful here? (The answer to several of my questions may well be "no", just wanted to be sure I raised them so we've thought it all through carefully.)

Copy link
Collaborator Author

@teunbrand teunbrand Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Claus, these are definitely helpful comments to think about. I guess the main thing I don't know if the transformation/reprojection of line segments describing a bounding box in {sf} just applies to the start/end coordinates or is interpolated somehow.
I'll try to answer some of the points, but for some I have to experiment a little bit more.

what this code is meant to do

It is supposed to provide a limits argument to ScaleContinuous$get_breaks(). The catch here is that the default calculated limits can be in a different metric (e.g. metres), whereas st_graticule() only takes in lon/lat break positions.

Should this respect the choice of default_crs in coord_sf()?

No, not currently. Because we're using the default st_graticule(..., datum = st_crs(4326)), only lon/lat degrees are sensible break positions.

Also, I'm worried about just transforming a bounding box.

Me too. I don't fully understand the nuances involved, which is why I appreciate the second set of eyes here.

Should that (calc_limits_bbox(), red.) be used here

That is a good thing to try out, but I have to find some examples where the results might diverge.

I added the function sf_transform_xy() to transform coordinates will imposing various checks. Is that useful here?

This is also a good thing to try out, but I might not have to use it if I use calc_limits_bbox().


if (!(is.waive(scale_x$breaks) && is.null(scale_x$n.breaks))) {
x_breaks <- scale_x$get_breaks(limits = bbox[c(1, 3)])
finite <- is.finite(x_breaks)
x_breaks <- if (any(finite)) x_breaks[finite] else NULL
}

if (!(is.waive(scale_y$breaks) && is.null(scale_y$n.breaks))) {
y_breaks <- scale_y$get_breaks(limits = bbox[c(2, 4)])
finite <- is.finite(y_breaks)
y_breaks <- if (any(finite)) y_breaks[finite] else NULL
}
}

list(x = x_breaks, y = y_breaks)
}

#' ViewScale from graticule
#'
Expand Down
48 changes: 48 additions & 0 deletions tests/testthat/_snaps/coord_sf/no-breaks.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
39 changes: 39 additions & 0 deletions tests/testthat/test-coord_sf.R
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,20 @@ test_that("graticule lines can be removed via theme", {
expect_doppelganger("no panel grid", plot)
})

test_that("graticule lines and axes can be removed via scales", {
skip_if_not_installed("sf")

df <- data_frame(x = c(1, 2, 3), y = c(1, 2, 3))
plot <- ggplot(df, aes(x, y)) +
geom_point() +
coord_sf() +
theme_gray() +
scale_x_continuous(breaks = NULL) +
scale_y_continuous(breaks = NULL)

expect_doppelganger("no breaks", plot)
})

test_that("axis labels are correct for manual breaks", {
skip_if_not_installed("sf")

Expand Down Expand Up @@ -300,6 +314,31 @@ test_that("sf_transform_xy() works", {

})

test_that("coord_sf() can use function breaks and n.breaks", {

polygon <- sf::st_sfc(
sf::st_polygon(list(matrix(c(-80, -76, -76, -80, -80, 35, 35, 40, 40, 35), ncol = 2))),
crs = 4326 # basic long-lat crs
)
polygon <- sf::st_transform(polygon, crs = 3347)

p <- ggplot(polygon) + geom_sf(fill = NA) +
scale_x_continuous(breaks = breaks_width(0.5)) +
scale_y_continuous(n.breaks = 4)

b <- ggplot_build(p)
grat <- b$layout$panel_params[[1]]$graticule

expect_equal(
vec_slice(grat$degree, grat$type == "E"),
seq(-81, -74.5, by = 0.5)
)
expect_equal(
vec_slice(grat$degree, grat$type == "N"),
seq(34, 40, by = 2)
)
})

test_that("coord_sf() uses the guide system", {
skip_if_not_installed("sf")
polygon <- sf::st_sfc(
Expand Down
Loading