-
-
Notifications
You must be signed in to change notification settings - Fork 91
[WIP] Grouped histogram (groupedhist) #319
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
Conversation
|
Can you say something about the difference between this and #315? |
|
Yes, sorry - I should have said more about this in my original post! I think results-wise, Implementation-wise, I've tried to make use of All that being said, I'm definitely open to trying to combine the functionality of the two recipes into one. I opened this second pull request mainly because I had already written most of the new code for use in a personal project, so I hope I haven't stepped on anyone's toes. |
|
You didn't step on anyone's toes, I think I like this better but will try to review in detail tomorrow. @piever want to have a look? |
|
Thanks! |
|
Nice work! But I think we should figure out the |
|
I definitely agree on wanting API consistency. In the commit 47f30e9 I just pushed, I got the This produces the |
7318c19 to
f348cab
Compare
|
@mkborregaard and @piever - is there anything else you think I should add or change? I understand also if you haven't had the chance to look at it, of course. Thanks again! |
|
Thanks for addressing the comments, it looks good to me! I'll merge in a couple of days. |
daschw
left a comment
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.
Looks great! Thanks a lot @pearlzli !
|
Thanks, everyone! |
|
Ahh I'm sorry, I just realized that I never updated the readme to reflect the |
|
It's already merged, so if you can make a new PR? |

Grouped histogram produced by calling
groupedbar. Usage:I couldn't figure out how to make this work using the
groupkeyword argument, which is why the group IDs are passed in as the second argument instead. Also, I recognize that there's some overlap between this recipe andstackedhistin #315. I'd love any and all feedback!