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.
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
Holed polygons #3128
Holed polygons #3128
Changes from 2 commits
60e4f48
858ca8f
2d8b1ff
1901c51
99f0166
5f6925f
5cdc09a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 don't think computing this value at build time is correct. Instead, I think you should just inline into
geom_polygon()
.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.