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

fix merging of histograms #1127

Merged
merged 1 commit into from
Aug 9, 2024
Merged

fix merging of histograms #1127

merged 1 commit into from
Aug 9, 2024

Conversation

Keksoj
Copy link
Member

@Keksoj Keksoj commented Aug 9, 2024

I have found that histograms comming from different workers were wrongly merged when producing one histograms for the whole of Sōzu.

Merely adding buckets like this:

| buckets le: | 0 | 1 | 3 | 7         | 15        |
|-------------|---|---|---|-----------|-----------|
| histogram A | 1 | 3 | 5 | (nothing) | (nothing) |
| histogram B | 0 | 2 | 4 | 8         | 13        |
| added       | 1 | 5 | 9 | 8         | 13        |

would yield counts that would decrease ot one point, if one histogram had less buckets than others.

We want this behaviour:

| buckets le: | 0 | 1 | 3 | 7         | 15        |
|-------------|---|---|---|-----------|-----------|
| histogram A | 1 | 3 | 5 | (nothing) | (nothing) |
| histogram B | 0 | 2 | 4 | 8         | 13        |
| added       | 1 | 5 | 9 | 13        | 18        |

This PR adds a unit test and fixes the behaviour.

@FlorentinDUBOIS
Copy link
Collaborator

Could you explain a bit more the issue? I do not understand the fix that you do.

@Keksoj
Copy link
Member Author

Keksoj commented Aug 9, 2024

Could you explain a bit more the issue? I do not understand the fix that you do.

adding up histograms gave wrong results when the number of buckets was different. The higher range of buckets would contain only the values of the longer histograms, instead of total values.

@Keksoj Keksoj merged commit 3474b9c into main Aug 9, 2024
25 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.

2 participants