-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Closes #48469 - Refactor and DRY up Kahan Sum algorithm #48558
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
Conversation
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
@elasticmachine test this please |
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! |
There was a problem hiding this 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
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. |
Cool! Thanks for your feedback. I have a busy week, but I'll work on this before finishing the week. |
Just a quick note that Kahan summation is also used in #47468 for the |
Add license Fix #48469 - Refactor and DRY up Kahan Sum algorithm
@elasticmachine update branch |
@elasticmachine test this please |
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! |
Great! Thank you for your feedback. I'll keep an eye on simple issues to keep contributing. Enjoy your conference 👍 |
@elasticmachine run elasticsearch-ci/packaging-sample-matrix |
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.