DOC: unambiguous np.histogram dtype description #25520
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As a follow-up of numpy#25403, let's be more specific than "dictates the dtype" because that could still mean that there is a mapping applied such as weights uint8 -> acc uint64, weights float -> acc double, ... That would make so much sense that people could expect that and thus read it into the documentation, I am afraid.
matheussouza88
approved these changes
Jan 2, 2024
ngoldbaum
reviewed
Jan 2, 2024
numpy/lib/_histograms_impl.py
Outdated
Comment on lines
730
to
731
| ``hist.dtype`` will be taken from `weights`, otherwise it will | ||
| hold large integers. |
There was a problem hiding this comment.
Suggested change
| ``hist.dtype`` will be taken from `weights`, otherwise it will | |
| hold large integers. | |
| ``hist.dtype`` will be taken from `weights`. |
Either delete this or rephrase a bit, "hold large integers" is ambiguous. I also think the explanation you've added in the weights description is sufficient, no need to repeat with different wording here.
There was a problem hiding this comment.
I looked at it again and just followed your suggestion / pushed an updated branch. (For a clean history, that second commit could be a "fixup" commit squashed into the first, but I don't know how the numpy merge process looks like.)
following ngoldbaum's suggestion, "large integers" was not a good description and not necessary here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a follow-up of #25403, let's be more specific than "dictates the
dtype" because that could still mean that there is a mapping applied
such as weights uint8 -> acc uint64, weights float -> acc double, ...
That would make so much sense that people could expect that and thus
read it into the documentation, I am afraid.
Skipping all CI because this is a documentation correction:
[skip ci]