Add Boxplot Aggregation by imotov · Pull Request #51948 · elastic/elasticsearch

imotov · 2020-02-05T14:56:41Z

Adds a boxplot aggregation that calculates min, max, medium and the first
and the third quartiles of the given data set.

Closes #33112

Adds a `boxplot` aggregation that calculates min, max, medium and the first and the third quartiles of the given data set. Closes elastic#33112

elasticmachine · 2020-02-05T14:56:44Z

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

polyfractal

Left some comments! Mainly around the docs, and a few bits and pieces elsewhere. This looks good!

polyfractal · 2020-02-05T22:18:55Z

docs/reference/aggregations/metrics/boxplot-aggregation.asciidoc

+[source,js]
+--------------------------------------------------
+{
+    "boxplot": {


I wonder, should we snake_case this to box_plot? I personally don't really like snake casing but it would be more consistent (date_histogram, significant_terms, etc).

Wikipedia states both spellings and boxplot was shorter, but I don't have strong feelings about it.

In that case, boxplot it is. I like shorter too

polyfractal · 2020-02-05T22:20:08Z

docs/reference/aggregations/metrics/boxplot-aggregation.asciidoc

+--------------------------------------------------
+{
+    "boxplot": {
+        "buckets_path": "my_cardinality_agg"


Typo re: "buckets_path" instead of "field"?

polyfractal · 2020-02-05T22:21:03Z

docs/reference/aggregations/metrics/boxplot-aggregation.asciidoc

+--------------------------------------------------
+// TESTRESPONSE[s/\.\.\./"took": $body.took,"timed_out": false,"_shards": $body._shards,"hits": $body.hits,/]
+
+As you can see, the aggregation will return a calculated value for each percentile


Looks like copy pasta leftovers :)

polyfractal · 2020-02-05T22:21:52Z

docs/reference/aggregations/metrics/boxplot-aggregation.asciidoc

+==== Script
+
+The boxplot metric supports scripting.  For example, if our load times
+are in milliseconds but we want percentiles calculated in seconds, we could use


"percentiles" typo

hmm, I am not sure how to call them here. Technically they are percentiles :) Values?

Whoops, yeah this is probably fine. Was thinking it was a copy/paste leftover.

polyfractal · 2020-02-05T22:23:29Z

docs/reference/aggregations/metrics/boxplot-aggregation.asciidoc

+
+<1> Compression controls memory usage and approximation error
+
+The TDigest algorithm uses a number of "nodes" to approximate percentiles -- the


I'm assuming this is the same as the Percentiles page? Maybe we should link to it instead? Or maybe there's a fancy way to embed a snippet from a different page? @nik9000 do you happen to know?

https://asciidoctor.org/docs/user-manual/#by-tagged-regions

polyfractal · 2020-02-05T22:23:50Z

server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java

            iw.addDocument(singleton(new NumericDocValuesField("number", 1)));
        }, (Consumer<InternalGlobal>) global -> {
-            assertEquals(1.0, global.getDocCount(), 2);
+            assertEquals(2, global.getDocCount());


polyfractal · 2020-02-05T22:33:35Z

...gin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregator.java

+import java.util.List;
+import java.util.Map;
+
+public class BoxplotAggregator extends NumericMetricsAggregator.MultiValue {


Out of curiosity, did you look into extending AbstractTDigestPercentilesAggregator? It might avoid some code duplication, but otoh it probably does more than you want/need. No strong feelings here.... the percentile/ranks code is already a complicated hierarchy so might be easier to just keep this separate.

I wonder if it'd be cleaner to make a class they could both delegate their common behavior to? It'd be cool to do that in a follow up change if it is tricky.

Yeah, noodled on this a bit and it's probably too much for this PR (especially with #51887 potentially in the works)

The main difficulty is that percentiles/ranks share TDigest and HDRHisto agg base classes, and this would just share with TDigest, so it'd be a fork in the tree in the middle somewhere.

AbstractTDigestBase --> AbstractTDigestPercentiles --> Percentiles \--> Ranks \--> Boxplot

Unsure if the extra complexity is worth it or not. 🤷‍♂️ But probably too much complexity for this PR.

I think it is up to @imotov but I'm fine waiting on it.

I looked at it when it was still IQR and it was immediate dead-end from the inheritance perspective because of IQR was a single value/multivalue agg. It might be a bit easier now but I agree with @nik9000 and still think we should do some refactoring by composition (instead of inheritance) here. However, I would prefer to do it in another PR.

polyfractal · 2020-02-05T22:39:04Z

...lugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/InternalBoxplot.java

+        if (super.equals(obj) == false) return false;
+
+        InternalBoxplot that = (InternalBoxplot) obj;
+        return Objects.equals(state, that.state);


Should we include format in this equality check?

Philosophically, I wonder if we should check equality of the boxplot components instead of the state? Like, are two boxplots identical if min/max/q1/q2/q3 are the same but the rest of the distribution is different?

I really dunno heh :)

It is already included in super.

🤦‍♂️

polyfractal · 2020-02-05T22:40:59Z

.../analytics/src/test/java/org/elasticsearch/xpack/analytics/boxplot/InternalBoxplotTests.java

+            expected.add(input.state());
+        }
+        assertNotNull(expected);
+        // The final calculated result may very depending on the order, which requires higher delta


Is this comment leftover? E.g. the delta looks like it's zero?

nik9000 · 2020-02-06T13:52:15Z

docs/reference/aggregations/metrics/boxplot-aggregation.asciidoc

+
+<1> Compression controls memory usage and approximation error
+
+The TDigest algorithm uses a number of "nodes" to approximate percentiles -- the


https://asciidoctor.org/docs/user-manual/#by-tagged-regions

nik9000 · 2020-02-06T13:54:55Z

x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/Boxplot.java

+
+import org.elasticsearch.search.aggregations.metrics.NumericMetricsAggregation;
+
+public interface Boxplot extends NumericMetricsAggregation.MultiValue {


Do we need these any more?

Hmm, I'm not sure tbh. We might want to keep them around as a bridge to the HLRC? E.g. InternalMin and ParsedMin both implement Min, and it might help us make sure they don't diverge?

Or maybe useless bloat now that the TC is gone and we can get rid of them... but probably better to investigate that in a separate PR and apply it to all aggs if we decide we can remove them?

The HLRC won't have Boxplot on the classpath because it doesn't have the analytics module. With top_metrics I just pointed folks to ParsedTopMetrics. I'm fine either way. It is just that when I made my new agg I figured I could save myself a little work an not make it.

Argh, you're right. Keep forgetting this is in xpack. 👍 probably not necessary given the current situation

nik9000 · 2020-02-06T14:47:35Z

...ytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java

+    BoxplotAggregationBuilder> {
+    public static final String NAME = "boxplot";
+
+    public static final ParseField COMPRESSION_FIELD = new ParseField("compression");


I wonder if it'd be better to use the field from the PercentilesAggregationBuilder. That at least is a hint that it means the same thing.

nik9000 · 2020-02-06T14:48:17Z

...ytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java

+    static {
+        PARSER = new ObjectParser<>(BoxplotAggregationBuilder.NAME);
+        ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false);
+        PARSER.declareDouble((builder, compression) -> builder.compression = compression, COMPRESSION_FIELD);


I think if you've declared the setter you may as well use it here.

So BoxplotAggregationBuilder::compression

Also that'd gt your validation too.

nik9000 · 2020-02-06T14:50:26Z

...gin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregator.java

+import java.util.List;
+import java.util.Map;
+
+public class BoxplotAggregator extends NumericMetricsAggregator.MultiValue {


I wonder if it'd be cleaner to make a class they could both delegate their common behavior to? It'd be cool to do that in a follow up change if it is tricky.

nik9000 · 2020-02-06T14:52:20Z

...lugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/InternalBoxplot.java

+
+    enum Metrics {
+
+        min, max, q1, q2, q3;


Could you name these in SHOUTING_SNAKE_CASE like normal Java enums? Is there a super compelling reason not to?

nik9000 · 2020-02-06T14:54:04Z

...lugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/InternalBoxplot.java

+    @Override
+    public double value(String name) {
+        Metrics metrics = Metrics.valueOf(name);
+        switch (metrics) {


Would you be making a private method on Metrics that takes this as a parameter and calls the right method instead?

nik9000 · 2020-02-06T14:54:32Z

...lugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/InternalBoxplot.java

+        return new InternalBoxplot(name, merged, format, pipelineAggregators(), metaData);
+    }
+
+    static class Fields {


I thought that these Fields objects had gone out of favor. But I'm not really in the loop.

…xplot-aggs

imotov · 2020-02-07T12:29:53Z

@elasticmachine test this please

jtibshirani · 2020-02-07T18:09:10Z

I was wondering if boxplot could be run on the histogram field type? If so we could mention it in the docs, otherwise it could be a nice future extension.

imotov · 2020-02-07T18:55:35Z

@jtibshirani excellent idea! It doesn't support it at the moment. I will open a follow up PR to add this support.

polyfractal

I'm happy with it, lgtm!

nik9000

Lgtm 🚢

Refactors BoxplotAggregator to reuse parts that are already available in AbstractTDigestPercentilesAggregator Follow up for elastic#51948

Relates: elastic/elasticsearch#51948 This commit implements the boxplot aggregation. Integration tests run against XPackCluster because it requires a license to use.

Add Boxplot Aggregation

e239ac5

Adds a `boxplot` aggregation that calculates min, max, medium and the first and the third quartiles of the given data set. Closes elastic#33112

imotov added >feature :Analytics/Aggregations Aggregations v8.0.0 v7.7.0 labels Feb 5, 2020

imotov requested a review from polyfractal February 5, 2020 14:56

$polyfractal$

polyfractal reviewed Feb 5, 2020

View reviewed changes

nik9000 reviewed Feb 6, 2020

View reviewed changes

imotov added 2 commits February 6, 2020 16:38

Merge remote-tracking branch 'elastic/master' into issue-33112-add-bo…

ec311ae

…xplot-aggs

Address review comments

caaec5a

$polyfractal$

polyfractal approved these changes Feb 7, 2020

View reviewed changes

wylieconlon mentioned this pull request Feb 7, 2020

Box plot / Candlestick visualization elastic/kibana#4157

Closed

nik9000 approved these changes Feb 7, 2020

View reviewed changes

imotov merged commit c50cfa0 into elastic:master Feb 7, 2020

imotov added the backport pending label Feb 7, 2020

imotov mentioned this pull request Feb 11, 2020

Add Boxplot Aggregation (7.x) #52174

Merged

imotov removed the backport pending label Feb 11, 2020

jpountz added the release highlight label Feb 20, 2020

imotov mentioned this pull request Mar 10, 2020

Clean up BoxplotAggregator to reuse AbstractTDigestPercentilesAggregator #53365

Closed

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

38 tasks

russcam mentioned this pull request Apr 15, 2020

Implement Boxplot aggregation elastic/elasticsearch-net#4592

Merged

imotov deleted the issue-33112-add-boxplot-aggs branch May 1, 2020 22:18

imotov mentioned this pull request Jul 30, 2020

Add additional box plot agg options #60466

Closed

jakelandis removed the v8.0.0 label Jul 26, 2021

jakelandis added the v8.0.0-alpha1 label Jul 26, 2021


		<1> Compression controls memory usage and approximation error

		The TDigest algorithm uses a number of "nodes" to approximate percentiles -- the


		import org.elasticsearch.search.aggregations.metrics.NumericMetricsAggregation;

		public interface Boxplot extends NumericMetricsAggregation.MultiValue {

Conversation

imotov commented Feb 5, 2020

Uh oh!

elasticmachine commented Feb 5, 2020

Uh oh!

polyfractal left a comment

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!

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!

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!

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!

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!

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!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov commented Feb 7, 2020

Uh oh!

jtibshirani commented Feb 7, 2020

Uh oh!

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal left a comment