-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
teunbrand
merged 11 commits into
tidyverse:main
from
teunbrand:sf_scale_function_breaks
May 20, 2024
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
a118bb8
Helper function for breaks
teunbrand f070f46
Use new breaks
teunbrand bc50154
NULL breaks discard gridlines
teunbrand a78fb00
Fix typo
teunbrand 793eb6e
Add tests
teunbrand a6dbc9e
fallback for problematic crs
teunbrand 8a2ea9d
Add news bullet
teunbrand 4cd5018
Fix `breaks = NULL` case
teunbrand 62658c2
Merge branch 'main' into sf_scale_function_breaks
teunbrand 296597b
replace removed `len0_null()`
teunbrand 7ddc6c3
remove duplicated news entry
teunbrand File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'd really like some more sf-experienced eyes on this bit
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.
@clauswilke you are perhaps the one best fitting the "sf-experienced" moniker :-)
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.
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
incoord_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 incalc_limits_bbox()
. Should that be used here, and the choice of the limit method respected (see argumentlims_method
incoord_sf()
)? Finally, I added the functionsf_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.)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.
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.
It is supposed to provide a
limits
argument toScaleContinuous$get_breaks()
. The catch here is that the default calculated limits can be in a different metric (e.g. metres), whereasst_graticule()
only takes in lon/lat break positions.No, not currently. Because we're using the default
st_graticule(..., datum = st_crs(4326))
, only lon/lat degrees are sensible break positions.Me too. I don't fully understand the nuances involved, which is why I appreciate the second set of eyes here.
That is a good thing to try out, but I have to find some examples where the results might diverge.
This is also a good thing to try out, but I might not have to use it if I use
calc_limits_bbox()
.