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

Updates to include count & density to fix the use of geom_hexbin #1688

Closed
wants to merge 13 commits into from
Closed

Updates to include count & density to fix the use of geom_hexbin #1688

wants to merge 13 commits into from

Conversation

mikebirdgeneau
Copy link

@mikebirdgeneau mikebirdgeneau commented Jul 29, 2016

Updates to include count & density to fix the use of geom_hexbin with ..density.. or ..count..

Fixes #1608.

@@ -31,6 +31,8 @@ hexBinSummarise <- function(x, y, z, binwidth, fun = mean, fun.args = list(), dr

# Convert to data frame
out <- as.data.frame(hexbin::hcell2xy(hb))
out$count = as.vector(hb@count)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please use <- instead of =?

Copy link
Member

Choose a reason for hiding this comment

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

Also I'm not sure that the code should go here - it's only needed for hexbin, not the other summary functions.

@hadley
Copy link
Member

hadley commented Aug 2, 2016

Can you please also add:

  • A bullet point to news
  • A unit test. Use the simplest possible dataset and get the computed data with layer_data()

@mikebirdgeneau
Copy link
Author

I've made the requested changes to the best of my ability (still new to pull requests and contributions to third party repos).

  • Replaced = with <-.
  • Added bullet in 'news'.
  • Added simple unit test for geom_hex()
  • Noticed that the value property retains the same output as count, so really only density was missing as far as I can tell.

As for your comment regarding if this is the right place for the code, that was why I didn't submit a pull request in the first place on #1608 - I thought there might have been an architectural reason for the change that I wasn't familiar enough with to make the fix.

Happy to help if this is useful, otherwise I'm equally happy if someone else has a better or more elegant way to resolve this. Cheers!

@hadley
Copy link
Member

hadley commented Aug 3, 2016

Can you move the code to StatBinhex$compute_group()? You'll need to save the results of hexBinSummarise() to a variable, and then modify that.

Otherwise looks great!

@mikebirdgeneau
Copy link
Author

Done. I've moved the code to StatBinhex$compute_group(). Thanks for pointing me in the right direction @hadley. If there's anything else on this, just let me know.


To confirm this did indeed resolve #1608 (in addition to the successful unit test), I ran the code in #1608 and here's the result, which looks good.

DF <- data.frame(x=rnorm(1000), y=rnorm(1000))
ggplot(DF) + geom_hex(aes(x=x, y=y, fill=..density..))

image

@@ -7,6 +7,9 @@ ggplot2 1.0.1
* Improvements to order of colours and legend display for continuous colour
scales extracted from colorbrewer palettes by `scale_*_distiller()` (@jiho, 1076)

* Restore functionality for use of `..density..` in
Copy link
Member

Choose a reason for hiding this comment

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

This should go at the top of NEWS.md (just underneath the version number heading)

@hadley
Copy link
Member

hadley commented Aug 5, 2016

Nearly there - thanks for all the hard work!

@@ -2,6 +2,9 @@
ggplot2 1.0.1
----------------------------------------------------------------

* Restore functionality for use of `..density..` in
Copy link
Member

Choose a reason for hiding this comment

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

Still the wrong file - needs to be in NEWS.md

@hadley hadley closed this in 56e2dad Aug 5, 2016
@hadley
Copy link
Member

hadley commented Aug 5, 2016

Thanks!

@mikebirdgeneau
Copy link
Author

No problem - thanks for your patience! (Appreciate you taking the time to 'teach' when it could be faster to simply resolve yourself.)

@lock
Copy link

lock bot commented Jan 18, 2019

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 18, 2019
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.

geom_hex no longer recognizes ..density.. in 2.1.0
2 participants