Compute inter-percentile range statistics in Histogram.

Compute inter-percentile range statistics in Histogram.

Currently, the only statistic that measures noise is std, but this is not
strictly applicable to non-normal distributions, such as most of our metrics.

Inter-quartile range is another measure of noise that is more broadly applicable.
This CL generalizes IQR to inter-percentile ranges:
histogram.customizeSummaryOptions({iprs: [
tr.b.Range.fromExplicitRange(0.25, 0.75), tr.b.Range.fromExplicitRange(0.1, 0.9),
]});

BUG=catapult:#3319

Review-Url: https://codereview.chromium.org/2746363003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4628b575b1d7baa7e729288f3397c0b75926881e

[benjhayden, 2017-03-14 20:34:26]

benjhayden@chromium.org changed reviewers:
+ eakuefner@chromium.org, tdresser@chromium.org

[benjhayden, 2017-03-14 20:34:27]

PTAL

[eakuefner, 2017-03-17 20:14:21]

lgtm; i think it's ultimately up to tim what we end up adding here, so defer to
him on the actual stats.

[tdresser, 2017-03-17 21:18:41]

Shall we wait for Deep to gather some data for us to look at before landing
this?

[benjhayden, 2017-03-17 21:52:43]

On 2017/03/17 at 21:18:41, tdresser wrote:
> Shall we wait for Deep to gather some data for us to look at before landing
this?

SGTM

[Liquan (Max) Gu, 2017-04-24 19:58:42]

maxlg@chromium.org changed reviewers:
+ maxlg@chromium.org

[Liquan (Max) Gu, 2017-04-24 19:58:43]

I have posted a report on the comparison of STD, MAD, AAD, IQR here:
https://docs.google.com/document/d/1cu0h94BiL9BoBMSvAjNCZk1g4kixJvBTyOrUkUY3T...

https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
File tracing/tracing/value/histogram.html (right):

https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
tracing/tracing/value/histogram.html:574: }
When this.numValues === 0, it will return a scalar whose {value: NaN} and fails
the test "assert.strictEqual(0, hist.getStatisticScalar('ipr_000_100').value);"

Do we want to return scalar whose {value: 0} or {value: NaN} or scalar ===
undefined when the histogram is empty?

[benjhayden, 2017-05-03 17:42:24]

On 2017/04/24 at 19:58:43, maxlg wrote:
> I have posted a report on the comparison of STD, MAD, AAD, IQR here:
https://docs.google.com/document/d/1cu0h94BiL9BoBMSvAjNCZk1g4kixJvBTyOrUkUY3T...
> 
>
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> File tracing/tracing/value/histogram.html (right):
> 
>
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> tracing/tracing/value/histogram.html:574: }
> When this.numValues === 0, it will return a scalar whose {value: NaN} and
fails the test "assert.strictEqual(0,
hist.getStatisticScalar('ipr_000_100').value);"
> 
> Do we want to return scalar whose {value: 0} or {value: NaN} or scalar ===
undefined when the histogram is empty?

I think we try to limit use of NaN because it's often a source of bugs, despite
the number of isNaN checks already in this directory.
The primary use-case is the dashboard. If the scalar is undefined, then the
dashboard might show gaps in the charts. OTOH, these statistics are naturally
zero when there's only 1 or 2 samples, so if we also use zero when the histogram
is empty, then that might cause ambiguity.
I'll ask Ethan what these statistics should look like in the dashboard when the
histogram is empty.
Either way, the Histogram class and the Statistics static class should exhibit
the same behavior.

[benjhayden, 2017-05-03 17:50:31]

On 2017/05/03 at 17:42:24, benjhayden_OOO wrote:
> On 2017/04/24 at 19:58:43, maxlg wrote:
> > I have posted a report on the comparison of STD, MAD, AAD, IQR here:
https://docs.google.com/document/d/1cu0h94BiL9BoBMSvAjNCZk1g4kixJvBTyOrUkUY3T...
> > 
> >
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> > File tracing/tracing/value/histogram.html (right):
> > 
> >
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> > tracing/tracing/value/histogram.html:574: }
> > When this.numValues === 0, it will return a scalar whose {value: NaN} and
fails the test "assert.strictEqual(0,
hist.getStatisticScalar('ipr_000_100').value);"
> > 
> > Do we want to return scalar whose {value: 0} or {value: NaN} or scalar ===
undefined when the histogram is empty?
> 
> I think we try to limit use of NaN because it's often a source of bugs,
despite the number of isNaN checks already in this directory.
> The primary use-case is the dashboard. If the scalar is undefined, then the
dashboard might show gaps in the charts. OTOH, these statistics are naturally
zero when there's only 1 or 2 samples, so if we also use zero when the histogram
is empty, then that might cause ambiguity.
> I'll ask Ethan what these statistics should look like in the dashboard when
the histogram is empty.
> Either way, the Histogram class and the Statistics static class should exhibit
the same behavior.

Offline discussion suggests using undefined for all statistic scalars that
cannot be computed when a histogram is empty. "count" should be 0, but all other
statistics should be undefined.

[Liquan (Max) Gu, 2017-05-03 17:55:56]

On 2017/05/03 17:50:31, benjhayden_OOO wrote:
> On 2017/05/03 at 17:42:24, benjhayden_OOO wrote:
> > On 2017/04/24 at 19:58:43, maxlg wrote:
> > > I have posted a report on the comparison of STD, MAD, AAD, IQR here:
>
https://docs.google.com/document/d/1cu0h94BiL9BoBMSvAjNCZk1g4kixJvBTyOrUkUY3T...
> > > 
> > >
>
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> > > File tracing/tracing/value/histogram.html (right):
> > > 
> > >
>
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> > > tracing/tracing/value/histogram.html:574: }
> > > When this.numValues === 0, it will return a scalar whose {value: NaN} and
> fails the test "assert.strictEqual(0,
> hist.getStatisticScalar('ipr_000_100').value);"
> > > 
> > > Do we want to return scalar whose {value: 0} or {value: NaN} or scalar ===
> undefined when the histogram is empty?
> > 
> > I think we try to limit use of NaN because it's often a source of bugs,
> despite the number of isNaN checks already in this directory.
> > The primary use-case is the dashboard. If the scalar is undefined, then the
> dashboard might show gaps in the charts. OTOH, these statistics are naturally
> zero when there's only 1 or 2 samples, so if we also use zero when the
histogram
> is empty, then that might cause ambiguity.
> > I'll ask Ethan what these statistics should look like in the dashboard when
> the histogram is empty.
> > Either way, the Histogram class and the Statistics static class should
exhibit
> the same behavior.
> 
> Offline discussion suggests using undefined for all statistic scalars that
> cannot be computed when a histogram is empty. "count" should be 0, but all
other
> statistics should be undefined.

I agree with it.

[tdresser, 2017-05-09 15:33:55]

On 2017/05/03 17:55:56, Liquan (Max) Gu wrote:
> On 2017/05/03 17:50:31, benjhayden_OOO wrote:
> > On 2017/05/03 at 17:42:24, benjhayden_OOO wrote:
> > > On 2017/04/24 at 19:58:43, maxlg wrote:
> > > > I have posted a report on the comparison of STD, MAD, AAD, IQR here:
> >
>
https://docs.google.com/document/d/1cu0h94BiL9BoBMSvAjNCZk1g4kixJvBTyOrUkUY3T...
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> > > > File tracing/tracing/value/histogram.html (right):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> > > > tracing/tracing/value/histogram.html:574: }
> > > > When this.numValues === 0, it will return a scalar whose {value: NaN}
and
> > fails the test "assert.strictEqual(0,
> > hist.getStatisticScalar('ipr_000_100').value);"
> > > > 
> > > > Do we want to return scalar whose {value: 0} or {value: NaN} or scalar
===
> > undefined when the histogram is empty?
> > > 
> > > I think we try to limit use of NaN because it's often a source of bugs,
> > despite the number of isNaN checks already in this directory.
> > > The primary use-case is the dashboard. If the scalar is undefined, then
the
> > dashboard might show gaps in the charts. OTOH, these statistics are
naturally
> > zero when there's only 1 or 2 samples, so if we also use zero when the
> histogram
> > is empty, then that might cause ambiguity.
> > > I'll ask Ethan what these statistics should look like in the dashboard
when
> > the histogram is empty.
> > > Either way, the Histogram class and the Statistics static class should
> exhibit
> > the same behavior.
> > 
> > Offline discussion suggests using undefined for all statistic scalars that
> > cannot be computed when a histogram is empty. "count" should be 0, but all
> other
> > statistics should be undefined.
> 
> I agree with it.

So we've done some investigation here, and think IQR is the right thing to land.

Let's go ahead and land this.

[benjhayden, 2017-05-09 16:09:25]

On 2017/05/09 at 15:33:55, tdresser wrote:
> On 2017/05/03 17:55:56, Liquan (Max) Gu wrote:
> > On 2017/05/03 17:50:31, benjhayden_OOO wrote:
> > > On 2017/05/03 at 17:42:24, benjhayden_OOO wrote:
> > > > On 2017/04/24 at 19:58:43, maxlg wrote:
> > > > > I have posted a report on the comparison of STD, MAD, AAD, IQR here:
> > >
> >
https://docs.google.com/document/d/1cu0h94BiL9BoBMSvAjNCZk1g4kixJvBTyOrUkUY3T...
> > > > > 
> > > > >
> > >
> >
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> > > > > File tracing/tracing/value/histogram.html (right):
> > > > > 
> > > > >
> > >
> >
https://codereview.chromium.org/2746363003/diff/20001/tracing/tracing/value/h...
> > > > > tracing/tracing/value/histogram.html:574: }
> > > > > When this.numValues === 0, it will return a scalar whose {value: NaN}
and
> > > fails the test "assert.strictEqual(0,
> > > hist.getStatisticScalar('ipr_000_100').value);"
> > > > > 
> > > > > Do we want to return scalar whose {value: 0} or {value: NaN} or scalar
===
> > > undefined when the histogram is empty?
> > > > 
> > > > I think we try to limit use of NaN because it's often a source of bugs,
> > > despite the number of isNaN checks already in this directory.
> > > > The primary use-case is the dashboard. If the scalar is undefined, then
the
> > > dashboard might show gaps in the charts. OTOH, these statistics are
naturally
> > > zero when there's only 1 or 2 samples, so if we also use zero when the
> > histogram
> > > is empty, then that might cause ambiguity.
> > > > I'll ask Ethan what these statistics should look like in the dashboard
when
> > > the histogram is empty.
> > > > Either way, the Histogram class and the Statistics static class should
> > exhibit
> > > the same behavior.
> > > 
> > > Offline discussion suggests using undefined for all statistic scalars that
> > > cannot be computed when a histogram is empty. "count" should be 0, but all
> > other
> > > statistics should be undefined.
> > 
> > I agree with it.
> 
> So we've done some investigation here, and think IQR is the right thing to
land.
> 
> Let's go ahead and land this.

Great! I'll rebase and clean this up and say PTAL to restart reviews.

[benjhayden, 2017-05-09 16:49:41]

PTAL

[eakuefner, 2017-05-11 20:41:34]

still lgtm when tim is happy.

[tdresser, 2017-05-11 21:03:12]

lgtm with nit.

https://codereview.chromium.org/2746363003/diff/40001/tracing/tracing/value/h...
File tracing/tracing/value/histogram.html (right):

https://codereview.chromium.org/2746363003/diff/40001/tracing/tracing/value/h...
tracing/tracing/value/histogram.html:133: ['std', true],
Should std and avg be default? In the long term, it would be nice if most folks
picked avg and std, OR median and iqr, for example.

https://codereview.chromium.org/2746363003/diff/40001/tracing/tracing/value/h...
tracing/tracing/value/histogram.html:779: if (stat === 'percentile' || stat ===
'iprs') {
It's not completely clear to me why these are excluded. Should the comment also
mention percentiles?

[benjhayden, 2017-05-11 21:24:15]

https://codereview.chromium.org/2746363003/diff/40001/tracing/tracing/value/h...
File tracing/tracing/value/histogram.html (right):

https://codereview.chromium.org/2746363003/diff/40001/tracing/tracing/value/h...
tracing/tracing/value/histogram.html:133: ['std', true],
On 2017/05/11 at 21:03:12, tdresser wrote:
> Should std and avg be default? In the long term, it would be nice if most
folks picked avg and std, OR median and iqr, for example.

We have vague plans to redo the default statistics when the UIs are a bit
farther along: 
https://github.com/catapult-project/catapult/issues/3435

https://codereview.chromium.org/2746363003/diff/40001/tracing/tracing/value/h...
tracing/tracing/value/histogram.html:779: if (stat === 'percentile' || stat ===
'iprs') {
On 2017/05/11 at 21:03:12, tdresser wrote:
> It's not completely clear to me why these are excluded. Should the comment
also mention percentiles?

Sorry, I'm not sure what you mean by "excluded".
percentile and iprs summaryOptions are arrays. percentile contains numbers, iprs
contains Range objects.
We want to copy arrays so that adding a percentile or ipr to one histogram
doesn't modify another histogram.
The Range objects in iprs should not be modified once they are passed to
Histogram, so it seems ok to copy them.

[tdresser, 2017-05-12 12:31:17]

https://codereview.chromium.org/2746363003/diff/40001/tracing/tracing/value/h...
File tracing/tracing/value/histogram.html (right):

https://codereview.chromium.org/2746363003/diff/40001/tracing/tracing/value/h...
tracing/tracing/value/histogram.html:779: if (stat === 'percentile' || stat ===
'iprs') {
On 2017/05/11 21:24:15, benjhayden wrote:
> On 2017/05/11 at 21:03:12, tdresser wrote:
> > It's not completely clear to me why these are excluded. Should the comment
> also mention percentiles?
> 
> Sorry, I'm not sure what you mean by "excluded".
> percentile and iprs summaryOptions are arrays. percentile contains numbers,
iprs
> contains Range objects.
> We want to copy arrays so that adding a percentile or ipr to one histogram
> doesn't modify another histogram.
> The Range objects in iprs should not be modified once they are passed to
> Histogram, so it seems ok to copy them.

Gotcha, this SGTM.

[benjhayden, 2017-05-12 19:28:22]

The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 19:28:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 19:52:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 19:52:33]

Dry run: This issue passed the CQ dry run.

[benjhayden, 2017-05-12 20:12:48]

The CQ bit was checked by benjhayden@chromium.org

[commit-bot: I haz the power, 2017-05-12 20:12:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 20:14:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494619968067880,
"parent_rev": "ff3b45d456482f2ac0282d5532a9d38fe3d031d8", "commit_rev":
"4628b575b1d7baa7e729288f3397c0b75926881e"}

[commit-bot: I haz the power, 2017-05-12 20:14:12]

Description was changed from

==========
Compute inter-percentile range statistics in Histogram.

Currently, the only statistic that measures noise is std, but this is not
strictly applicable to non-normal distributions, such as most of our metrics.

Inter-quartile range is another measure of noise that is more broadly
applicable.
This CL generalizes IQR to inter-percentile ranges:
histogram.customizeSummaryOptions({iprs: [
tr.b.Range.fromExplicitRange(0.25, 0.75), tr.b.Range.fromExplicitRange(0.1,
0.9),
]});

BUG=catapult:#3319
==========

to

==========
Compute inter-percentile range statistics in Histogram.

Currently, the only statistic that measures noise is std, but this is not
strictly applicable to non-normal distributions, such as most of our metrics.

Inter-quartile range is another measure of noise that is more broadly
applicable.
This CL generalizes IQR to inter-percentile ranges:
histogram.customizeSummaryOptions({iprs: [
tr.b.Range.fromExplicitRange(0.25, 0.75), tr.b.Range.fromExplicitRange(0.1,
0.9),
]});

BUG=catapult:#3319

Review-Url: https://codereview.chromium.org/2746363003
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-05-12 20:14:13]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...