-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix various geom_sf related issues #2216
Conversation
All cases were in sf.R a geometry column is address with x$geometry, ggplot2 made the wrong assumption that the geometry column has a fixed name. I replaced this in certain instances, where the data are already pretty transformed and no longer have properties of sf objects, with a fixed position, i.e. x[[1]], which seems to work.
R/sf.R
Outdated
@@ -61,6 +61,10 @@ | |||
#' @name ggsf | |||
NULL | |||
|
|||
geom_column = function(data) { | |||
data[[ which(sapply(data, function(x) inherits(x, "sfc")))[1] ]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use vapply()
here instead of sapply()
? You should be able to drop the function()
too.
(And <-
instead of =
)
R/sf.R
Outdated
@@ -189,8 +193,9 @@ CoordSf <- ggproto("CoordSf", CoordCartesian, | |||
} | |||
|
|||
for (layer_data in data) { | |||
geometry <- layer_data$geometry | |||
if (is.null(geometry)) | |||
if (inherits(layer_data, "sf")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use {}
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth defining is.sf()
? We use inherits(x, "sf")
in a few places
R/sf.R
Outdated
}) | ||
}, | ||
|
||
transform = function(self, data, panel_params) { | ||
data$geometry <- sf_rescale01( | ||
data$geometry, | ||
data[[ which(sapply(data, function(x) inherits(x, "sfc")))[1] ]] <- sf_rescale01( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe geom_column()
should return an index so you can use it on the LHS and RHS here?
I think adding |
If you want me to add tests to address the last check, please let me know where. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests/testthat/test-geom-sf.R
Outdated
# Visual tests ------------------------------------------------------------ | ||
|
||
test_that("geom_sf draws correctly", { | ||
library(sf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since sf is only suggests, this should start with skip_if_not_installed("sf")
I think it's slightly nicer to not attach the package, but just sf::read_sf()
if (length(w) == 0) { | ||
"geometry" # avoids breaks when objects without geometry list-column are examined | ||
} else { | ||
# this may not be best in case more than one geometry list-column is present: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if length(w) > 1
, pick the first, but issue a warning?
tests/testthat/test-geom-sf.R
Outdated
|
||
test_that("geom_sf draws correctly", { | ||
library(sf) | ||
f = system.file("gpkg/nc.gpkg", package="sf") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<-
😄
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/ |
see description of individual commits