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

Ensure our histogram uses a non-zero step #113

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

godlygeek
Copy link
Contributor

@godlygeek godlygeek commented Mar 7, 2024

When all allocations are the same size, we were falling back to the size of the smallest allocation as our step for generating histogram buckets. When the smallest allocation was zero bytes, that would lead to a division by zero and an exception.

Fix this by falling back to 1 instead of the size of the smallest allocation. This will never lead to a zero division error, and still puts every element in the same bucket because they're all equal (otherwise we wouldn't have needed the fallback).

Closes #93

@godlygeek godlygeek requested a review from pablogsal March 7, 2024 00:01
@godlygeek godlygeek self-assigned this Mar 7, 2024
@godlygeek
Copy link
Contributor Author

Say @pablogsal - is it intentional that our histogram is space-separated? For 5 bins, we're printing:

▄   ▄ █ ▄

when I would have expected:

▄ ▄█▄

When all allocations are the same size, we were falling back to the size
of the smallest allocation as our step for generating histogram buckets.
When the smallest allocation was zero bytes, that would lead to
a division by zero and an exception.

Fix this by falling back to 1 instead of the size of the smallest
allocation. This will never lead to a zero division error, and still
puts every element in the same bucket because they're all equal
(otherwise we wouldn't have needed the fallback).

Signed-off-by: Matt Wozniski <[email protected]>
@pablogsal
Copy link
Member

Say @pablogsal - is it intentional that our histogram is space-separated? For 5 bins, we're printing:

I think it's not intentional :S

This makes it easier to read the histogram.

Signed-off-by: Matt Wozniski <[email protected]>
@godlygeek
Copy link
Contributor Author

I think it's not intentional :S

OK, pushed a second commit that removes the spaces 😅

@pablogsal pablogsal merged commit 26b5ac6 into bloomberg:main Mar 7, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running pytest with --most-allocations throws an ZeroDivisionError: float divmod()
2 participants