-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add moving average function #82122
Add moving average function #82122
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
c81a924
to
ffe33db
Compare
ffe33db
to
4906e24
Compare
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 compared this to the Elasticsearch behavior and I think we should change it slightly to behave the same and also simplify it when doing so:
- If there is a value (0 or other truthy value) in the current row, calculate moving average based on the current window, then add the to the window and cut off tail if necessary
- If there is no value (null or undefined) in the current row, set the output column to undefined as well, then proceed with the next row
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
}, | ||
{ inputColumnId: 'val', outputColumnId: 'output', window: 3 } | ||
); | ||
expect(result.rows.map((row) => row.output)).toEqual([ |
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 tried to replicate this using Elasticsearch and it seems like it's doing something different regarding null values. This is what I tried (maybe I have made a mistake in here):
Click to expand!
DELETE timeindex
POST timeindex/_doc
{
"ts": "2020-09-30"
}
POST timeindex/_doc
{
"ts": "2020-10-01"
}
POST timeindex/_doc
{
"ts": "2020-10-02"
}
POST timeindex/_doc
{
"ts": "2020-10-03",
"val": 1
}
POST timeindex/_doc
{
"ts": "2020-10-04",
"val": 2
}
POST timeindex/_doc
{
"ts": "2020-10-05",
"val": 0
}
POST timeindex/_doc
{
"ts": "2020-10-06"
}
POST timeindex/_doc
{
"ts": "2020-10-07",
"val": 0
}
POST timeindex/_doc
{
"ts": "2020-10-08"
}
POST timeindex/_doc
{
"ts": "2020-10-09",
"val": 0
}
POST timeindex/_doc
{
"ts": "2020-10-10",
"val": 8
}
POST timeindex/_doc
{
"ts": "2020-10-11",
"val": 0
}
POST timeindex/_search
{
"size": 0,
"aggs": {
"time": {
"date_histogram": {
"field": "ts",
"interval": "day"
},
"aggs": {
"sum": {
"avg": {
"field": "val"
}
},
"deri": {
"moving_fn": {
"buckets_path": "sum",
"window": 3,
"script": "MovingFunctions.unweightedAvg(values)"
}
}
}
}
}
}
Running this results in the following series:
Values | Moving avg | Expression function |
---|---|---|
undefined | undefined | undefined |
undefined | undefined | undefined |
undefined | undefined | undefined |
1 | undefined | undefined |
2 | 1.0 | 1 |
0 | 1.5 | 1.5 |
undefined | undefined | 1 |
0 | 1.0 | 1 |
undefined | undefined | 0 |
0 | 0.66666 | 0 |
8 | 0 | 0 |
0 | 2.6666 | 4 |
It seems like the major difference is:
- Elasticsearch: If an undefined value is encountered, skip the current row and do not change the window
- Expression function: If an undefined value is encountered, calculate the moving average based on the existing window and add "undefined" to window (which might push real values out)
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.
Behavior looks good to me, but it seems like the algorithm can be simplified a bit based on the recent changes
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
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.
LGTM, code review only (but the unit test suite is convincing 🙂 ) Left three nits about further simplifications of the logic
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
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.
Code LGTM, thanks for the detailed docs & tests!
src/plugins/expressions/common/expression_functions/specs/moving_average.ts
Outdated
Show resolved
Hide resolved
292ab64
to
25b9073
Compare
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
Built on top of #81178
Related to #61777
This adds a moving average expression function similar to the existing cumulative sum and (in process of reviewing) derivative function.
Similar to #80129 and #81178