-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fixes #12926 - inconsistency upon passing C in hexbin #26113
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
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Thank you! I think this makes sense from what you described (and the results). Can you please add a test for this? For example, a matplotlib/lib/matplotlib/tests/test_axes.py Lines 2655 to 2660 in 3c597f6
|
Thanks for taking a look, @oscargus I've added a test |
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 good! Thanks!
Anyone can merge on green CI |
Thanks @krooijers and congrats on your first merged PR in Matplotlib. Hope to see you around! |
@oscargus @ksunden This issue was discussed at length in #16865 and #21381. There's at least one issue with the fix there: this makes the reducer (defaulting to np.mean) be called for empty arrays too, and that triggers a warning ("Mean of empty slice") as soon as a cell is empty (e.g., As argued in the linked threads, I think we should switch the default reduce_C_function to np.sum (because it usually makes more sense than np.mean), and possibly also deprecate support for mincnt when C is not passed (because in that case mincnt is more simply and more generally expressed as vmin (plus possibly setting a custom "under" value on the colormap)). |
PR summary
Closes #12926
Around line 4998 in _axes.py two code paths diverge, depending on whether parameter
C
is given. The strict inequality betweenaccum
andmincnt
in L5003 whenC == None
is not symmetric to the strict inequality at L5017, whenC != None
. This causes data points to become "hidden", when the number of data points (accum
) is equal tomincnt
.'Tis but a scratch: changing
> mincnt
to>= mincnt
was enough to get consistent hexbin plots.PR checklist