New Matrix Stats Aggregation module by nknize · Pull Request #18300 · elastic/elasticsearch

nknize · 2016-05-12T17:59:15Z

This PR adds a new aggs-matrix-stats module. The module introduces a new class of aggregations called Matrix Aggregations. Matrix aggregations work on multiple fields and produce a matrix as output. The matrix aggregation provided by this module is the matrix_stats aggregation which computes the following descriptive statistics over a set of fields:

Covariance
Correlation

For completeness (and interpretation purposes) the following per-field statistics are also provided:

sample count
population mean
population variance
population skewness
population kurtosis

Example request:

"aggs": {
    "matrixstats": {
        "matrix_stats": {
            "field": ["poverty", "income"]
        }
     }
}

Example response:

{
    "aggregations": {
        "matrixstats": {
            "fields": [{
                "name": "income",
                "count": 50,
                "mean": 51985.1,
                "variance": 7.383377037755103E7,
                "skewness": 0.5595114003506483,
                "kurtosis": 2.5692365287787124,
                "covariance": {
                    "income": 7.383377037755103E7,
                    "poverty": -21093.65836734694
                },
                "correlation": {
                    "income": 1.0,
                    "poverty": -0.8352655256272504
                }
            }, {
                "name": "poverty",
                "count": 50,
                "mean": 12.732000000000001,
                "variance": 8.637730612244896,
                "skewness": 0.4516049811903419,
                "kurtosis": 2.8615929677997767,
                "covariance": {
                    "income": -21093.65836734694,
                    "poverty": 8.637730612244896
                },
                "correlation": {
                    "income": -0.8352655256272504,
                    "poverty": 1.0
                }
            }]
        }
    }
}

This PR replaces #16826. The differences include:

renames aggregation from multifield_stats to matrix_stats
adds aggregation code as a module instead of directly to core
refactors Integration Tests as REST Tests

nknize · 2016-05-13T18:46:19Z

This PR has been updated to include MultiValuesSource classes (per comment in #18285)

@colings86, @jpountz could one (or both) of you have a look if you get a chance?

colings86 · 2016-05-16T08:59:43Z

core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregation.java

I think this should actually be in ValuesSourceAggregatorBuilder since this CommonFields interface is for the response side of the aggregations but value_type is for the request side, so it doesn't really belong here

Ah, so is InternalAggregation specifically for the response side? I'm not as intimate with the aggs framework so I thought it was just an internal framework class (request / response agnostic).

So then what do you think about having a CommonFields static class to AggregatorBuilder for common fields used on the request side?

+1 to having CommonFields class in AggregatorBuilder

colings86 · 2016-05-16T09:17:47Z

@nknize I left some comments on the aggregation type stuff but I don't know enough about the actual statistics we are calculating here to review those bits. Maybe @brwe or @polyfractal would be able to review those bits?

jpountz · 2016-05-16T12:50:29Z

...tats/src/main/java/org/elasticsearch/search/aggregations/matrix/MatrixAggregationPlugin.java

should we rather do it in onModule? cc @colings86

polyfractal · 2016-05-16T15:42:33Z

I tried, and largely failed, to grok the stats calculations. My math skills are pretty rusty and the streaming formulas look different from what I'm used to. I'd try to get a second opinion from @brwe :)

nknize · 2016-05-23T19:00:03Z

Great suggestions! I incorporated all feedback and the PR is ready for another review. Here are the main changes:

Added MultiValueMode support for handling multi value fields (it was easier adding it here instead of postponing to a later PR)
Added a MultiValuesSource class to encapsulate field names and ValuesSource arrays, this is now used to retrieve document field values instead of creating a Map inside of collect
Updated doReduce to merge all shard results in an empty RunningStats object.
Added Rest testing for MultiValueMode support

/cc @colings86, @jpountz

jpountz · 2016-05-25T21:46:44Z

...rix-stats/src/main/java/org/elasticsearch/search/aggregations/matrix/stats/RunningStats.java

to make this class more efficient, we might want to give an ordinal to every field and then use field ordinals as keys rather than field names? (I don't mind it being done as a follow-up, I'm just suggesting)

+1 for followup PR. I'll open an issue

nknize · 2016-05-26T21:43:37Z

Thanks for the latest round of review @jpountz. I went ahead and made the minor code changes, added support for missing values, and updated documentation. Let me know what you think.

jpountz · 2016-05-31T09:00:51Z

docs/reference/modules/aggregations/matrix/stats.asciidoc

s/field/fields/

jpountz · 2016-05-31T09:12:00Z

I'd like @colings86 to have a final look, at least for the core refactorings that you did. But otherwise this looks good to me for a first iteration!

colings86 · 2016-05-31T09:29:30Z

core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java

Should we make all the fields of this class private now we have setters/getters for them so there is only one way of accessing/modifying the values? (maybe in a followup PR?)

I went ahead and did it in this PR but will keep it as a separate commit when I merge.

colings86 · 2016-05-31T09:35:33Z

@jpountz @nknize I took a look at the core changes and they LGTM. I also took a brief look at the module, there are unit and REST tests but we are missing integration tests for this. Is there a reason we have not done an integration tests for this aggregation? All other aggregations have integration tests.

nknize · 2016-05-31T16:07:51Z

@colings86 I had asked @rjernst about integration testing in modules. I could be wrong but I think to avoid the overhead of starting/stopping integration test clusters in plugins and modules the ESIntegTestCase tests should be replaced by simple Unit and Rest tests? It was a bit tricky but what I did was refactor the integration tests (defined by AbstractNumericTestCase) from the original PR (#16826) to the yaml Rest definitions included in this PR. I also split out testing of RunningStats and MatrixStatsResults into a separate RunningStatsTests unit test class to ensure the RunningStats (and corresponding results) are computed as expected.

rjernst · 2016-06-01T19:47:29Z

there are unit and REST tests but we are missing integration tests for this

REST tests are real integration tests. No need for fantasy land tests.

This commit adds a new aggs-matrix-stats module. The module presents a new class of aggregations called Matrix Aggregations. Matrix aggregations work on multiple fields and produce a matrix as output. The first matrix aggregation provided by the module is matrix_stats aggregation. This aggregation computes the following statistics over a set of fields: * Covariance * Correlation For completeness (and interpretation purposes) the following per-field statistics are also provided: * sample count * population mean * population variance * population skewness * population kurtosis

…stats agg module

nknize added >feature review discuss WIP :Analytics/Aggregations Aggregations labels May 12, 2016

clintongormley mentioned this pull request May 13, 2016

[WIP] MultiField_Stats Aggregation #16826

Closed

nknize force-pushed the feature/MatrixStats-Module branch from f3b1159 to bce9bbf Compare May 13, 2016 18:37

nknize mentioned this pull request May 13, 2016

Add MultiValuesSource support to Aggregation Framework #18285

Closed

nknize assigned colings86 May 13, 2016

colings86 reviewed May 16, 2016
View reviewed changes

jpountz reviewed May 16, 2016
View reviewed changes

nknize force-pushed the feature/MatrixStats-Module branch from 89d8c39 to 2fa3320 Compare May 23, 2016 18:48

nknize added v5.0.0-alpha3 and removed discuss WIP labels May 24, 2016

jpountz reviewed May 25, 2016
View reviewed changes

nknize force-pushed the feature/MatrixStats-Module branch from 2a811b5 to f33b04e Compare May 27, 2016 18:58

jpountz reviewed May 31, 2016
View reviewed changes

docs/reference/modules/aggregations/matrix/stats.asciidoc Outdated

Copy link
Copy Markdown

Contributor

jpountz May 31, 2016

Choose a reason for hiding this comment

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

s/field/fields/

colings86 reviewed May 31, 2016
View reviewed changes

nknize force-pushed the feature/MatrixStats-Module branch from f33b04e to 2f66665 Compare May 31, 2016 16:26

nknize force-pushed the feature/MatrixStats-Module branch 4 times, most recently from b85644a to 1706782 Compare June 1, 2016 21:38

nknize removed the review label Jun 1, 2016

nknize unassigned colings86 Jun 1, 2016

nknize added 3 commits June 1, 2016 16:39

Adding MultiValuesSource support classes and documentation to matrix …

90b8f5d

…stats agg module

set ValuesSourceConfig fields as private

54575e5

nknize force-pushed the feature/MatrixStats-Module branch from 1706782 to 54575e5 Compare June 1, 2016 21:39

nknize merged commit 54575e5 into elastic:master Jun 1, 2016

nknize mentioned this pull request Jun 1, 2016

Improve Matrix Agg RunningStats performance by using field ordinals as hash keys #18691

Closed

clintongormley mentioned this pull request Jun 23, 2016

Correlation Metric Aggregator #16817

Closed

colings86 mentioned this pull request Aug 4, 2016

Should we remove/modify some of the experiment tags in the documentation #19798

Closed

vielmetti mentioned this pull request Jun 20, 2017

Preliminary support for ARM #25318

Merged

nknize mentioned this pull request Jul 10, 2019

Annotated Heatmap of a Correlation Matrix elastic/elastic-charts#263

Closed

$@polyfractal$ polyfractal mentioned this pull request Mar 3, 2020

Removing experimental status of Matrix Stats #53065

Closed

Conversation

nknize commented May 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nknize commented May 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented May 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polyfractal commented May 16, 2016

Uh oh!

nknize commented May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nknize commented May 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz commented May 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented May 31, 2016

Uh oh!

nknize commented May 31, 2016

Uh oh!

rjernst commented Jun 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nknize commented May 12, 2016 •

edited

Loading

nknize commented May 13, 2016 •

edited

Loading

nknize commented May 23, 2016 •

edited

Loading