-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Better sf support #3164
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
Better sf support #3164
Conversation
This fixes #2718 at last. Thanks Thomas! |
This looks good to me in principle, but I don't have time this week to go over it in detail (proofing my book this week, and likely reformatting many of the figures). I think increasing the required sf version is good anyways, because otherwise axis ticks may be placed in the wrong location due to a pernicious bug that was only recently fixed. |
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.
Minor comments.
By the way, it's not necessarily included in this PR, but can we add some simple tests about the gpar()
is built as intended?
R/geom-sf.R
Outdated
} | ||
geometry <- x$geometry | ||
type <- sf_types[sf::st_geometry_type(geometry)] | ||
is_point <- type %in% 'point' |
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.
Use "
, not '
, as the style guide says: https://style.tidyverse.org/syntax.html#quotes
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.
Oops - nice catch. My life is a constant struggle between my own preference and Hadleys ;-)
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.
Looks fine to me.
type <- sf_types[sf::st_geometry_type(geometry)] | ||
is_point <- type %in% 'point' | ||
type_ind <- match(type, c('point', 'line', 'other')) | ||
defaults <- list( |
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.
We will need to re-think this at some point in order to expose to the user in the same way as other geoms. But we don't need to do that in this PR.
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/ |
This PR adapts
geom_sf
andcoord_sf
to make use of the improvements in performance introduced in sf v0.7-3. The changes are as follows:st_normalize
during rescaling to greatly speed up `coord_sfsf
decide on how/if to split it up to multiple grobsThis PR introduces a new version dependency on sf as it does not make much sense to support both implementations simultaneously