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 moving function linear weighted avg #118516

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

Quentin18
Copy link
Contributor

@Quentin18 Quentin18 commented Dec 11, 2024

Closes #113751

There is a problem in the MovingFunctions.linearWeightedAvg method. The totalWeight is initialized to 1 whereas it should be initialized to 0.

In the issue example, the raw values are:

  • 15.3984375
  • 15.3984375
  • 15.703125

Actually, the Linear Weighted Moving Average (LWMA) is calculated as follows:

$$ LWMA=\frac{\sum v_i\times w_i}{1+\sum w_i} $$

Where $v=\{15.3984375, 15.3984375, 15.703125\}$ and $w=\{1, 2, 3\}$. The extra 1 in the denominator is due to the wrong initialization. In the example:

$$ LWMA=\frac{15.3984375\times 1+15.3984375\times 2+15.703125\times 3}{1+1+2+3}=13.329241071 $$

The LWMA formula should be:

$$ LWMA=\frac{\sum v_i\times w_i}{\sum w_i} $$

In the example:

$$ LWMA=\frac{15.3984375\times 1+15.3984375\times 2+15.703125\times 3}{1+2+3}=15.55078125 $$

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Dec 11, 2024
@benwtrent benwtrent added :Analytics/Aggregations Aggregations and removed needs:triage Requires assignment of a team area label labels Dec 12, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 12, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@nik9000 nik9000 self-assigned this Dec 12, 2024
@nik9000 nik9000 added auto-backport Automatically create backport pull requests when merged v8.18.0 >bug labels Dec 12, 2024
@nik9000
Copy link
Member

nik9000 commented Dec 12, 2024

@elasticmcahine, test this please.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Thanks @Quentin18! I've kicked this off with the robots and will get back to you. I imagine there might be a test failure, but the robots will find it.

@iverase
Copy link
Contributor

iverase commented Dec 12, 2024

@elasticmachine, test this please.

@nik9000
Copy link
Member

nik9000 commented Dec 12, 2024

Community PR needs a changelog YAML file at docs/changelog/118516.yaml

Looks like I do that myself.

@nik9000
Copy link
Member

nik9000 commented Dec 12, 2024

@elasticsearchmachine, test this please

@john-wagster
Copy link
Contributor

@elasticmachine test this please

@nik9000
Copy link
Member

nik9000 commented Dec 13, 2024

I've pushed a few test fixes that ought to make the robots happy. It's an integration test, and it's forwards and backwards compatibility form.

@elasticsearchmachine, test this please.

@nik9000
Copy link
Member

nik9000 commented Dec 13, 2024

@elasticmachine test this please

@nik9000
Copy link
Member

nik9000 commented Dec 13, 2024

I have a spelling problem calling the right robot....

@nik9000
Copy link
Member

nik9000 commented Dec 13, 2024

@elasticmachine test this please

I had to merge main in. Again. BWC tests were sad.

@nik9000 nik9000 merged commit 908cf9a into elastic:main Dec 13, 2024
17 checks passed
@nik9000
Copy link
Member

nik9000 commented Dec 13, 2024

Ok robot, are you going to make backport commits or should I?

@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.17 Commit could not be cherrypicked due to conflicts
8.x Commit could not be cherrypicked due to conflicts
7.17 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 118516

@Quentin18 Quentin18 deleted the fix-moving-fn-linear-weighted-avg branch December 13, 2024 22:38
@nik9000
Copy link
Member

nik9000 commented Dec 14, 2024

💔 Backport failed

Ok robot. Monday, monday.

@iverase
Copy link
Contributor

iverase commented Dec 16, 2024

@nik9000 I will take care of the backports

iverase pushed a commit to iverase/elasticsearch that referenced this pull request Dec 16, 2024
Fix moving function linear weighted avg

# Conflicts:
#	modules/aggregations/build.gradle
@nik9000
Copy link
Member

nik9000 commented Dec 16, 2024

@nik9000 I will take care of the backports

Thanks @iverase !

iverase added a commit that referenced this pull request Dec 16, 2024
Fix moving function linear weighted avg

Co-authored-by: Quentin Deschamps <[email protected]>
iverase added a commit to iverase/elasticsearch that referenced this pull request Dec 16, 2024
)

Fix moving function linear weighted avg

Co-authored-by: Quentin Deschamps <[email protected]>
# Conflicts:
#	server/src/main/java/org/elasticsearch/rest/action/search/SearchCapabilities.java
iverase added a commit that referenced this pull request Dec 16, 2024
Fix moving function linear weighted avg

Co-authored-by: Quentin Deschamps <[email protected]>
# Conflicts:
#	server/src/main/java/org/elasticsearch/rest/action/search/SearchCapabilities.java
@iverase iverase removed the v7.17.27 label Dec 16, 2024
maxhniebergall pushed a commit to maxhniebergall/elasticsearch that referenced this pull request Dec 16, 2024
)

Fix moving function linear weighted avg

Co-authored-by: Quentin Deschamps <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations auto-backport Automatically create backport pull requests when merged backport pending >bug external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.17.1 v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moving Function linearWeightedAvg consistently under-reports value
6 participants