Hi@RebeccaWPerry, maybe I'm mistaken, but I think the problem in#20877 is that ifc is provided, each element in a hexagon can count for less than 1. This has nothing to do with the less than being inclusive or not.

@jklymak it was super interesting to look into! Check out the example I added -- where C is all ones. Surely if I'm "scaling" by 1 there should be no difference?

if C is none: hexagons with less than mincount are set to nan (code)

if C is provided: hexagons with more than mincount are set to the value (code)

And I found no evidence that the scaling of C should impact the count -- count is simply count as implemented and as the bug reporter expected.

I don't know about the scaling, but the condition onmincntis inconsistent, and scaling doesn't make a difference there.

This would need a test though.

And I found no evidence that the scaling of C should impact the count -- count is simply count as implemented and as the bug reporter expected.

Agreed, I originally misdiagnosed the problem.

This change is fine, but as@QuLogic suggests a test would be nice. You can probably justnp.testing.assert_allclose(c1, c2) at the end of your code above and add where hex bin is tested (probably just intest_axes.py.

Sounds good - I'll add a test!

This PR is affected by a re-writing of our history to remove a large number of accidentally committed filessee discourse for details.

To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote"upstream"

git remote updategit checkout maingit merge --ff-only upstream/maingit checkout YOUR_BRANCHgit rebase --onto=main upstream/old_master# git rebase -i main # if you prefergit push --force-with-lease# assuming you are tracking your branch

If you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you.

Thank you for your contributions to Matplotlib and sorry for the inconvenience.

I think I have addressed the feedback -- added a test and rebased to main. Please let me know if there is anything else to address!

While I agree with the change (which probably needs an API change note), there is an additional issue to consider (#16865 (comment)): with this PR,hexbin(np.random.randn(10000), np.random.randn(10000), C=np.random.rand(10000)) (for example) now emits "RuntimeWarning: Mean of empty slice." and "RuntimeWarning: invalid value encountered in double_scalars", because the default reduce_C_function (np.mean()) is getting called on empty bins as well. At least I think the default reduce_C_function should not emit the warning (it should be a small wrapper around np.mean, that directly returns nan with no warning for empty inputs). Perhaps the doc for reduce_C_function should also mention that the reducer needs to be able to handle empty inputs.

A quick thought (I've not followed the situation in detail, so might be wrong):

Perhaps the doc for reduce_C_function should also mention that the reducer needs to be able to handle empty inputs.

Or shouldwe wrap everyreduce_C_function that is passed. That'd take the burden from the user and will allow passing many existing functions as is. The only way this could fall short is if the answer to empty inputs should not be NaN.

But I think(?) the problem also comes from the fact that the default reducer is actually a bit weird; e.g. I would rather have expected it to be sum() rather than mean(). For example

hexbin(np.random.randn(10000), np.random.randn(10000), C=np.random.rand(10000), reduce_C_function=np.sum)

likely makes more sense (I know, that's pretty vague...) than

hexbin(np.random.randn(10000), np.random.randn(10000), C=np.random.rand(10000))

and sum() is just fine with empty inputs (and I'd rather have sum() return 0 rather than nan).

(Not that the current situation is really better, I guess... but my point is that auto-wrapping is not a complete solution either.)

Yes, I agree, this probably has to be fixed before this can go in.

re-posting my message on#16865 (let's keep the discussion here instead)

Briefly looking at this again, I think that

• when C is unset (plain counts), it makes sense to default to <0 (i.e., keep everything): this ensures that empty bins are reported as zero and not nan, which can be helpful e.g. for interactive cursoring.
• when C is set, OTOH, it makes sense to skip empty bins and set them to nan, as one cannot compute means on them anyways.

So mincnt has two slightly different semantics in both cases. Perhaps yet another solution would be to note that mincnt is actually not so useful in the case of C being unset: in that case, the empty bins can just be reported as zero, and if you want to mask them out you can do so using standard colormapping tools (e.g., vmin=1 and cmap=mpl.cm.get_cmap().with_extremes(under="none")). So perhaps one should just deprecate the use of mincnt together with C unset? I'm not really sure what the history behind mincnt is... (unfortunately the git log is not helpful here)

I'm parsing my way through the various options here - in response to this:

and sum() is just fine with empty inputs (and I'd rather have sum() return 0 rather than nan).

I think it's important for sum() to return nan when where are no points in a cell because no points is different than having points that sum to 0 (either all points are 0, or there are negative and positive values that balance each other out).

@anntzer thanks for flagging this interconnected issue. Please bear with me since this is my first PR that has gotten this far.
Here are the use cases under consideration and current behavior:
Weights C NOT specified

• mincount not specified --> returns 0's for cells with 0 points
• mincount specified --> output array has no values for cells
  with < mincnt points (output array is actually smaller, not full of nan's)

Weights C ARE specified

• mincount not specified --> mincnt=0, mean throws warning, only returns values
  where aggregator function could be computed (output array is smaller than total cells, not full of nan's)
• mincount specified --> array has no values for cells with < mincnt points, still throws
  warning for mean if mincnt=0

Here are the paths forward I can see in the thread above and from thinking about it some more:

1. Change default to mincount=1. Change docstring to mincount >= 0 to show that a user can specify 0 to include cells with 0 counts (or for other reduction functions that seamlessly handle 0 points)

2. Change default min aggregator function to sum() so that the default behavior does not throw warnings. If someone switches to mean() they would have to navigate the warning message/specifiy mincnt=1 to avoid warnings. Could add something to that effect in the doc string?

3. In the code, check if reduction function handles 0 pts gracefully or not and set mincnt to 0 or 1 accordingly (if not explicitly set). This seems like confusing magic under the hood, so I would probably not recommend it.

4. Wrapper around np.mean that returns nan without warning. Mention in doc that reducer needs to be able to handle 0 points.

Am I missing any viable options? Any recommendations on which path forward to choose? I like 1 or 2, but I'm not sure how to weigh the importance of maintaining the same default behavior of a function from one code release to the next.

Thanks in advance for the guidance! I'm happy to make the changes if I can get a clear sense of what the best direction to take this in is!

You ran into a fairly complicated issue to start with, and you are doing well in keeping all relevant points in your head :-)

I agree with your description of the current behavior. We also try very hard not to change behavior,especially if there's no deprecation warning in between (silently changing the plots of end users is not nice). Here, I think the agreement is that we want mincnt to behave consistently regardless of whether C is given, so there will need to be a backcompat break at some point, and therefore a deprecation warning. I also agree that the magic of (3) is not really a good option (we can't know what warnings we need to suppress, and what warnings are legitimate ones that need to be propagated out).

In my view, defaulting reduce_C_function to mean() doesn't make sense, just like hist2d defaults to summing weights that fall into a bin, rather than averaging them. Given that we're already going to have a deprecation period for the change in mincnt semantics, we may just as well do both of them at once, and both switch to sum() and to inclusive mincnt. (This is something where I'd like other devs to weigh in, in particular@dstansby, who has recently played with hexbin().) Therefore, I guess the deprecation would be something like (unchecked...)

_unset = object()def hexbin(..., mincnt=_unset, reduce_C_function=_unset):    ...    if C is None:  # old code is fine; normalize _unset to zero    if C is not None:        if reduce_C_function is _unset:            # normalize to mean-wrapped-with-zero-handling, but warn that this            # will use sum() in the future instead.        # do as previously        # if mincnt is set and any bin has exactly mincnt points, warn that the        # semantics will change, but note that one can set mincnt=-1 to "easily"        # suppress the warning

and in the future (after the deprecation period), even though the signature will now be

def hexbin(..., mincnt=0, reduce_C_function=np.sum):

still add a special-case forreduce_C_function is np.mean to wrap it with warning-suppression.

I don't think its as complicated as all that. Currently if C is set, and a bin is empty, the bin is set to NaN. The suggested change would simply be:

iflen(vals)>=mincntandlen(vals)>0:lattice1[i,j]=reduce_C_function(vals)else:lattice1[i,j]=np.NaN

That doesn't require a deprecation, and is consistent with the docs. It does require an API note, since it is fixing a bug?

Popping to draft until the review comments can be dealt with. Feel free to mark active when ready...

Hi@RebeccaWPerry, thank you for all your work digging into this. It looks like the issue you were targeting has recently been fixed by another pull request (by basically the same approach). The fix was#26113, with some follow up in#27179 - a pretty gnarly problem!

So I'm going to close this one.