Skip to content
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

Use the isoband package to calculate contour lines and bands #3439

Merged
merged 11 commits into from
Jul 16, 2019

Conversation

paleolimbot
Copy link
Member

This is a PR to address #3044. While the desired behaviour (calculating contours at draw time that can be labeled) can't be done immediately because Geoms don't have access to non-position scales, a good start is to use @clauswilke's isoband package to calculate contour lines (as opposed to grDevices::contourLines()) in stat_contour(). The isoband package also has a function to return contour polygons, which are implemented in this PR as stat_contour_filled() and geom_contour_filled().

Additional refactoring steps were:

  • The complete parameter is not used in the current calculation and hasn't been used for about 4 years (estimated from the git blame view), so I removed it.
  • I added function parameters and documentation for bins, binwidth and breaks. These appeared in examples but were not function arguments and were not documented.

Things I am not sure about:

  • The calculation of breaks in this way is different than the calculation of bins and breaks everywhere else in ggplot2, and I wonder if they should be updated in this PR. In particular, letting breaks be a function I think would be helpful.
  • I'm not sure about how I implemented geom_contour_filled(). There is no GeomContourFilled, it just calls layer() with stat = "contour_filled" and geom = "polygon".
  • I'm not sure what the corner cases are for isoband::iso(band|contour), but I think I should probably add more tests to make sure inappropriate/weird inputs are handled properly (such as for Proposal: Warning when using density2d + coord_map #2702).

@paleolimbot
Copy link
Member Author

A few more notes:

  • isoband output isn't stable in SVG (see Refactor geom_contour()? #3044 (comment) ), so it can't be visually tested. I've tested all the parts non-visually and added an independent test for polygons, which were being tested using stat_density2d() previously.

@paleolimbot paleolimbot requested a review from hadley July 13, 2019 13:57
@clauswilke
Copy link
Member

I had a quick look and didn't see anything that concerns me. I'll be traveling extensively over the next two weeks, starting today. Therefore, you probably shouldn't wait for a more careful review from me, unless there's anything you feel uncertain about.

@paleolimbot
Copy link
Member Author

I appreciate you taking a look! When ggplot2 is ready for scale access at draw time, I imagine I will have many more questions!

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're in here, it would be useful to quickly profile this code, just to see if there are any major bottlenecks we could improve.

DESCRIPTION Outdated Show resolved Hide resolved
@lock
Copy link

lock bot commented Jan 12, 2020

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/

@lock lock bot locked and limited conversation to collaborators Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants