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

[DOCS] Fix the weighed average documentation #81307

Conversation

salvatore-campagna
Copy link
Contributor

@salvatore-campagna salvatore-campagna commented Dec 3, 2021

This inconsistency has been found while working on #81306.

The documentations states that if the weight field is missing, and no
explicit missing configuration is provided, a default value of 1 is used.
This is incorrect and does not match the implementation of the weighted
average aggregator. In this specific case the document is skipped, instead.

The documentations states that if the `weight` field is missing, and no
explicit missing configuration is provided, a default value of 1 is used.
This is incorrect and does not match the implementation of the weighted
average aggregator. In this specific case the document is skipped, instead.
@salvatore-campagna salvatore-campagna added the >docs General docs changes label Dec 3, 2021
@elasticmachine elasticmachine added the Team:Docs Meta label for docs team label Dec 3, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Thanks @salvatore-campagna. I left a suggestion that fixes up some incorrect text surrounding your change. Let me know what you think.

Comment on lines 210 to 215
The `missing` parameter defines how documents that are missing a value should be treated.
The default behavior is different for `value` and `weight`:

By default, if the `value` field is missing the document is ignored and the aggregation moves on to the next document.
If the `weight` field is missing, it is assumed to have a weight of `1` (like a normal average).
By default, if the `value` or `weight` field is missing the document is ignored and the aggregation moves on to the next document.

Both of these defaults can be overridden with the `missing` parameter:
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the text surrounding this change is still incorrect. For example:

The default behavior is different for value and weight:

is still wrong.

I left a suggestion that fixes up the surrounding text and shortens this a bit. I also updated the text to mention null values, which are treated the same as missing values based on my testing. Feel free to change the wording or let me know what you think.

Suggested change
The `missing` parameter defines how documents that are missing a value should be treated.
The default behavior is different for `value` and `weight`:
By default, if the `value` field is missing the document is ignored and the aggregation moves on to the next document.
If the `weight` field is missing, it is assumed to have a weight of `1` (like a normal average).
By default, if the `value` or `weight` field is missing the document is ignored and the aggregation moves on to the next document.
Both of these defaults can be overridden with the `missing` parameter:
By default, the aggregation excludes documents with a missing or `null` value
for the `value` or `weight` field. Use the `missing` parameter to specify a
default value for these documents instead.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@salvatore-campagna salvatore-campagna merged commit 2b5ebba into elastic:master Dec 3, 2021
salvatore-campagna added a commit to salvatore-campagna/elasticsearch that referenced this pull request Dec 7, 2021
The documentations states that if the `weight` field is missing, and no
explicit missing configuration is provided, a default value of 1 is used.
This is incorrect and does not match the implementation of the weighted
average aggregator. In this specific case the document is skipped, instead.
salvatore-campagna added a commit to salvatore-campagna/elasticsearch that referenced this pull request Dec 7, 2021
The documentations states that if the `weight` field is missing, and no
explicit missing configuration is provided, a default value of 1 is used.
This is incorrect and does not match the implementation of the weighted
average aggregator. In this specific case the document is skipped, instead.
salvatore-campagna added a commit to salvatore-campagna/elasticsearch that referenced this pull request Dec 7, 2021
The documentations states that if the `weight` field is missing, and no
explicit missing configuration is provided, a default value of 1 is used.
This is incorrect and does not match the implementation of the weighted
average aggregator. In this specific case the document is skipped, instead.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.0
7.16
7.15

@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.15

elasticsearchmachine pushed a commit that referenced this pull request Dec 7, 2021
The documentations states that if the `weight` field is missing, and no
explicit missing configuration is provided, a default value of 1 is used.
This is incorrect and does not match the implementation of the weighted
average aggregator. In this specific case the document is skipped, instead.
elasticsearchmachine pushed a commit that referenced this pull request Dec 7, 2021
The documentations states that if the `weight` field is missing, and no
explicit missing configuration is provided, a default value of 1 is used.
This is incorrect and does not match the implementation of the weighted
average aggregator. In this specific case the document is skipped, instead.
elasticsearchmachine pushed a commit that referenced this pull request Dec 7, 2021
The documentations states that if the `weight` field is missing, and no
explicit missing configuration is provided, a default value of 1 is used.
This is incorrect and does not match the implementation of the weighted
average aggregator. In this specific case the document is skipped, instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >docs General docs changes Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Docs Meta label for docs team v7.15.3 v7.16.1 v8.0.0-rc1 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants