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

Hexbin mincnt parameter docstring should say "more than or equal to" not "more than" #16865

Closed
tdpetrou opened this issue Mar 21, 2020 · 7 comments · Fixed by #27179
Closed

Comments

@tdpetrou
Copy link
Contributor

The current docstring for hexbin has this for mincnt:

mincnt : int > 0, default: *None*
    If not *None*, only display cells with more than *mincnt*
    number of points in the cell.

But, it really displays cells that have more than or equal to mincnt.

The int > 0 is also slightly confusing. It does accept 0 and negative numbers and can appear to relate to the 'more than' below when it does not.

@anntzer
Copy link
Contributor

anntzer commented Mar 21, 2020

There's also the problem that when C is None, the condition is >= mincnt (as you write), but when C is not None, the condition is > mincnt (as documented). Perhaps this should be made consistent first? (I think >= mincnt makes more sense?)

@tdpetrou
Copy link
Contributor Author

Agreed, consistency should be done first. >= makes more sense to me as well as it is a minimum count.

@anntzer
Copy link
Contributor

anntzer commented Mar 21, 2020

Actually, looking at this again, I think I see why it's > for the case with a reducer: with the effective mincnt default of 0 (None is converted to 0), we don't want to get a warning with np.mean(empty_list), but just get nan straight away (at least np.mean([]) returns nan with a warning, but np.max() would be even worse as it raises an exception).

On the other hand, in the case without C, it makes sense to keep empty cells mapped, well, to zero.

Not sure if there's an easy way out there...

@RebeccaWPerry
Copy link
Contributor

Why is the default to set mincnt to 0 instead of 1? Would someone want to run a reduce function on a cell containing 0 points? As documented, mincnt must be greater than 0 (mincnt : int > 0), so it seems counterintuitive that the code sets it to 0.

@anntzer
Copy link
Contributor

anntzer commented Oct 25, 2021

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)

@github-actions
Copy link

This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear. Thanks for your help!

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jul 14, 2023
@github-actions github-actions bot added the status: closed as inactive Issues closed by the "Stale" Github Action. Please comment on any you think should still be open. label Aug 14, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 14, 2023
@anntzer
Copy link
Contributor

anntzer commented Aug 15, 2023

#26113 (comment) is likely still a problem for now.

@anntzer anntzer reopened this Aug 15, 2023
@github-actions github-actions bot removed status: closed as inactive Issues closed by the "Stale" Github Action. Please comment on any you think should still be open. status: inactive Marked by the “Stale” Github Action labels Aug 16, 2023
@QuLogic QuLogic added this to the v3.8.1 milestone Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants