-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Holed polygons #3128
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
Holed polygons #3128
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
60e4f48
Add support for holed polygons using an optional subgroup aesthetic
thomasp85 858ca8f
Fix temporary overwrite of support flag
thomasp85 2d8b1ff
Small style fixes suggested by Claus
thomasp85 1901c51
Move grid version check inside draw_panel
thomasp85 99f0166
Add rule argument to geom_polygon
thomasp85 5f6925f
make last polygon example conditional on grid version
thomasp85 5cdc09a
Add news bullet
thomasp85 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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Other geoms check this assumption and issue a warning if an aesthetic changes where it’s not supposed to. See e.g.
geom_area()
. Needed 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.
I think that may become prohibitly expensive to check for the kind of data that people will throw at this... Maybe make it more clear in the docs?
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.
Do you think a single call to
unique()
is that much more expensive than, say, the ordering of the data that is also happening?ggplot2/R/geom-ribbon.r
Lines 80 to 83 in 18be30e
At a minimum, this might be worth a careful benchmark on a very large dataset, such as the output from
isobands()
on a large raster image.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’ll happily do the benchmark, but the big difference is that geom_ribbon uses a draw_group method whereas geom_polygon uses draw_panel. This means that geom_polygon would have to split up the data and call unique on each sub-data.frame all for the sake of a possible warning. For geom ribbon the split has already been done so it is rather cheap
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 see. In any case, it's your call. I won't insist.
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 may wanted to eventually tackle this problem by providing a new geom where there's one row per polygon and the vertices are stored in a nested data frame.