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.

Sign up for GitHub

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

Jump to bottom

Closes #48469 - Refactor and DRY up Kahan Sum algorithm #48558

Merged
merged 3 commits into from
Nov 11, 2019
Merged

Closes #48469 - Refactor and DRY up Kahan Sum algorithm #48558

merged 3 commits into from
Nov 11, 2019

Conversation

JosemyDuarte
Copy link
Contributor

I have created CompensatedSum to centralize the logic of Kahan Sum and replace it in all the classes mentioned by #48469.

I haven't put the looping logic inside the class since there are places where we do other stuff besides the sum in the loop, like in StatsAggregator where besides calculating the sum we also get the max and min value.

@dnhatn dnhatn added the :Analytics/Aggregations Aggregations label Oct 28, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@not-napoleon
Copy link
Member

@elasticmachine test this please

@JosemyDuarte
Copy link
Contributor Author

Hi @not-napoleon! I just saw the build log that seems to be failing and I am not sure if I understand how my change could have caused that. If this is my fault I'm willing to solve it, but I would appreciate it if you can point me in the right direction.

Thanks!

not-napoleon
not-napoleon requested changes Oct 28, 2019
View reviewed changes
Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the solution overall. Left a couple of clean up comments that need to be fixed before we can merge it.

I appreciate you doing this work. In the future though, please check if someone else has already asked to work on an issue before you start on it.

server/src/main/java/org/elasticsearch/search/aggregations/metrics/CompensatedSum.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/CompensatedSum.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/CompensatedSum.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/CompensatedSum.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/CompensatedSum.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalWeightedAvg.java Show resolved Hide resolved
@not-napoleon
Copy link
Member

I agree, the BWC failure looks unrelated. I'll run it again after the requested changes land, and if it's green then we should be okay.

@JosemyDuarte
Copy link
Contributor Author

Cool! Thanks for your feedback. I have a busy week, but I'll work on this before finishing the week.

@csoulios
Copy link
Contributor

csoulios commented Nov 1, 2019

Just a quick note that Kahan summation is also used in #47468 for the string_stats aggregation

@not-napoleon
Copy link
Member

@elasticmachine update branch

@not-napoleon
Copy link
Member

@elasticmachine test this please

@not-napoleon
Copy link
Member

Hi @JosemyDuarte

This looks good, thanks for making those changes. I'm at a conference this week, so may be slow in checking in, but I'll keep an eye on the tests. I'll let you know if there are failures I need you to look into. Not sure I'll have time to merge it this week, but if I don't, I definitely will next week.

Thanks again for the contribution!

@JosemyDuarte
Copy link
Contributor Author

Great! Thank you for your feedback. I'll keep an eye on simple issues to keep contributing. Enjoy your conference 👍

@not-napoleon
Copy link
Member

@elasticmachine run elasticsearch-ci/packaging-sample-matrix

@not-napoleon not-napoleon merged commit eb70849 into elastic:master Nov 11, 2019
@not-napoleon not-napoleon mentioned this pull request Nov 11, 2019
Merged
not-napoleon pushed a commit to not-napoleon/elasticsearch that referenced this pull request Nov 11, 2019
@JosemyDuarte @not-napoleon
Refactor and DRY up Kahan Sum algorithm (elastic#48558)
3fb13a5
not-napoleon added a commit that referenced this pull request Nov 11, 2019
@not-napoleon
Refactor and DRY up Kahan Sum algorithm (#48558) (#48959)
d9e5692
@talevy talevy mentioned this pull request Nov 22, 2019
Closed
@polyfractal polyfractal mentioned this pull request Nov 25, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not-napoleon not-napoleon not-napoleon approved these changes

Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations >refactoring v7.6.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JosemyDuarte @elasticmachine @not-napoleon @csoulios @jakelandis @dnhatn