Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to enable the possibility of streaming rather than holding onto the whole list in memory. Also, we change the API for getting spikes so that now it takes a list of objects together with a valuation function, rather than taking a list of pairs (of objects with their values).

BUG=chromium:644406
Committed: https://chromium.googlesource.com/infra/infra/+/27d3c91752defc8bf2556775462f4aa90419e74a

[Sharu Jiang, 2016-09-08 00:48:24]

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

[Sharu Jiang, 2016-09-08 00:48:25]

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/detect_regression_range.py (right):

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:153:
historic_metadata[-max_win_size:], ['chrome_version', 'cpm'])
We don't need this anymore.

We can just use:
spikes = GetSpikes(historic_metadata, lambda x: x['cpm'])

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:163: return spikes[-1][0]
I think this should be

return spikes[-1][0]['chrome_version'], spikes[-1][1]['chrome_version']

[wrengr (wrong one), 2016-09-08 18:12:13]

Removed the obviated GetAttributesListFromHistoricData. Still needs to have the
unit tests updated before being ready for review.

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/detect_regression_range.py (right):

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:153:
historic_metadata[-max_win_size:], ['chrome_version', 'cpm'])
On 2016/09/08 00:48:24, sharu jiang wrote:
> We don't need this anymore.
> 
> We can just use:
> spikes = GetSpikes(historic_metadata, lambda x: x['cpm'])

Right. I just hadn't gotten around to deleting the
GetAttributesListFromHistoricData stuff yet. (As I mentioned, right now this
particular CL is just to explain what I meant, not actually ready for review. I
also need to update the unit tests to reflect the change in GetSpike's type.

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:163: return spikes[-1][0]
On 2016/09/08 00:48:24, sharu jiang wrote:
> I think this should be
> 
> return spikes[-1][0]['chrome_version'], spikes[-1][1]['chrome_version']

Do you mean you want DetectRegressionRange to (continue to) return a pair of the
'chrome_version' strings, rather than returning a pair of historic metadata
objects? Either way is fine by me, it's just that you'd mentioned about wanting
to get other information out of the historic metadata objects.

[Sharu Jiang, 2016-09-08 18:26:08]

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/detect_regression_range.py (right):

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:153:
historic_metadata[-max_win_size:], ['chrome_version', 'cpm'])
On 2016/09/08 18:12:13, wrengr wrote:
> On 2016/09/08 00:48:24, sharu jiang wrote:
> > We don't need this anymore.
> > 
> > We can just use:
> > spikes = GetSpikes(historic_metadata, lambda x: x['cpm'])
> 
> Right. I just hadn't gotten around to deleting the
> GetAttributesListFromHistoricData stuff yet. (As I mentioned, right now this
> particular CL is just to explain what I meant, not actually ready for review.
I
> also need to update the unit tests to reflect the change in GetSpike's type.

Acknowledged.

https://codereview.chromium.org/2325503002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:163: return spikes[-1][0]
On 2016/09/08 18:12:13, wrengr wrote:
> On 2016/09/08 00:48:24, sharu jiang wrote:
> > I think this should be
> > 
> > return spikes[-1][0]['chrome_version'], spikes[-1][1]['chrome_version']
> 
> Do you mean you want DetectRegressionRange to (continue to) return a pair of
the
> 'chrome_version' strings, rather than returning a pair of historic metadata
> objects? Either way is fine by me, it's just that you'd mentioned about
wanting
> to get other information out of the historic metadata objects.

The final result of DetectRegressionRange should still be chrome_version pair,
the other information is used later (need experiments) to get this final result
:)

[wrengr (wrong one), 2016-09-08 18:44:16]

Description was changed from

==========
detect_regression_range.py: switching from pairs to valuation functions.

BUG=
==========

to

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so not it takes a list of
objects plus a valuation function, rather than taking a list of pairs of objects
with their values.

This CL is currently dependent on https://codereview.chromium.org/2311393003/,
but really we want to abandon that CL and just land this one instead.

BUG=chromium:644406
==========

[wrengr (wrong one), 2016-09-08 18:45:32]

Description was changed from

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so not it takes a list of
objects plus a valuation function, rather than taking a list of pairs of objects
with their values.

This CL is currently dependent on https://codereview.chromium.org/2311393003/,
but really we want to abandon that CL and just land this one instead.

BUG=chromium:644406
==========

to

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs of
objects with their values.

This CL is currently dependent on https://codereview.chromium.org/2311393003/,
but really we want to abandon that CL and just land this one instead.

BUG=chromium:644406
==========

[wrengr (wrong one), 2016-09-08 18:46:21]

Description was changed from

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs of
objects with their values.

This CL is currently dependent on https://codereview.chromium.org/2311393003/,
but really we want to abandon that CL and just land this one instead.

BUG=chromium:644406
==========

to

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs (of
objects with their values).

This CL is currently dependent on https://codereview.chromium.org/2311393003/,
but really we want to abandon that CL and just land this one instead.

BUG=chromium:644406
==========

[wrengr (wrong one), 2016-09-08 20:15:53]

Description was changed from

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs (of
objects with their values).

This CL is currently dependent on https://codereview.chromium.org/2311393003/,
but really we want to abandon that CL and just land this one instead.

BUG=chromium:644406
==========

to

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs (of
objects with their values).

This CL obviates https://codereview.chromium.org/2311393003/

BUG=chromium:644406
==========

[wrengr (wrong one), 2016-09-08 20:15:54]

wrengr@google.com changed reviewers:
+ aarya@google.com, mbarbella@chromium.org, stgao@chromium.org

[wrengr (wrong one), 2016-09-08 20:16:43]

Description was changed from

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs (of
objects with their values).

This CL obviates https://codereview.chromium.org/2311393003/

BUG=chromium:644406
==========

to

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs (of
objects with their values).

BUG=chromium:644406
==========

[wrengr (wrong one), 2016-09-08 20:17:52]

Description was changed from

==========
Reorganizing detect_regression_range.py

We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs (of
objects with their values).

BUG=chromium:644406
==========

to

==========
We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs (of
objects with their values).

BUG=chromium:644406
==========

[wrengr (wrong one), 2016-09-08 20:18:24]

Description was changed from

==========
We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects plus a valuation function, rather than taking a list of pairs (of
objects with their values).

BUG=chromium:644406
==========

to

==========
We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects together with a valuation function, rather than taking a list of
pairs (of objects with their values).

BUG=chromium:644406
==========

[wrengr (wrong one), 2016-09-08 20:28:49]

This is a new CL for cleaning up our detection of regression ranges. The
difference vs the old CL (https://codereview.chromium.org/2311393003/) is that
now we use a valuation function rather than requiring objects to be paired up
with their values. This CL is a lot cleaner overall so I'm hoping to land it in
lieu of the old one.

PTAL

[Sharu Jiang, 2016-09-08 21:33:14]

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/detect_regression_range.py (right):

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:23: """Given a time series,
determine the regression ranges for each
nit: A convention, for long doc string, keep the first sentence one line.

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/detect_regression_range_test.py (right):

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/detect_regression_range_test.py:25: e0 = ('1', 0.5)
Shouldn't this be {'chrome_version': '1', 'cpm': 0.5}?
This test will break.

[wrengr (wrong one), 2016-09-08 22:28:50]

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/detect_regression_range_test.py (right):

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/detect_regression_range_test.py:25: e0 = ('1', 0.5)
On 2016/09/08 21:33:14, sharu jiang wrote:
> Shouldn't this be {'chrome_version': '1', 'cpm': 0.5}?
> This test will break.

The GetSpikes function takes a list of whatever type of objects the caller
likes. The DetectRegressionRange function chooses to have the objects be
historic metadata dicts, but that's not required. For this test we choose the
objects to be pairs where the value is the second component of the pair.

I ran the tests and this works as written.

[Sharu Jiang, 2016-09-08 23:55:19]

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/detect_regression_range_test.py (right):

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/detect_regression_range_test.py:25: e0 = ('1', 0.5)
On 2016/09/08 22:28:50, wrengr (wrong one) wrote:
> On 2016/09/08 21:33:14, sharu jiang wrote:
> > Shouldn't this be {'chrome_version': '1', 'cpm': 0.5}?
> > This test will break.
> 
> The GetSpikes function takes a list of whatever type of objects the caller
> likes. The DetectRegressionRange function chooses to have the objects be
> historic metadata dicts, but that's not required. For this test we choose the
> objects to be pairs where the value is the second component of the pair.
> 
> I ran the tests and this works as written.

Ok, didn't notice the snd function...

[Sharu Jiang, 2016-09-09 17:33:30]

lgtm with nit in previous comments.

[wrengr (wrong one), 2016-09-09 17:41:48]

The CQ bit was checked by wrengr@google.com

[commit-bot: I haz the power, 2016-09-09 17:42:02]

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

[commit-bot: I haz the power, 2016-09-09 17:42:03]

This CL has an open dependency (Issue 2306823002 Patch 140001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[stgao, 2016-09-09 18:13:59]

lgtm with nits on style and docstrings.

Once fixed, feel free for CQ.

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/detect_regression_range.py (right):

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:23: """Given a time series,
determine the regression ranges for each
nit: one line summary here.

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:27: function for computing the
"value" of each event. The events themselves
Information in this paragraph could be merged into the docstrings for arguments
below.

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
appengine/findit/crash/detect_regression_range.py:89: repr(event), spikiness)
nit: could it fix in one line?

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
File appengine/findit/crash/test/detect_regression_range_test.py (right):

https://codereview.chromium.org/2325503002/diff/60001/appengine/findit/crash/...
appengine/findit/crash/test/detect_regression_range_test.py:24: snd = lambda x:
x[1]
style nit: usually we avoid abbreviation.

[wrengr (wrong one), 2016-09-09 20:02:24]

The CQ bit was checked by wrengr@google.com

[wrengr (wrong one), 2016-09-09 20:02:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
katesonia@chromium.org, stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2325503002/#ps70001
(title: "Addressing nits")

[commit-bot: I haz the power, 2016-09-09 20:02:29]

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, 2016-09-09 20:16:27]

Description was changed from

==========
We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects together with a valuation function, rather than taking a list of
pairs (of objects with their values).

BUG=chromium:644406
==========

to

==========
We avoid using array indices, to avoid potential off-by-one errors as well as to
enable the possibility of streaming rather than holding onto the whole list in
memory. Also, we change the API for getting spikes so that now it takes a list
of objects together with a valuation function, rather than taking a list of
pairs (of objects with their values).

BUG=chromium:644406

Committed:
https://chromium.googlesource.com/infra/infra/+/27d3c91752defc8bf2556775462f4...
==========

[commit-bot: I haz the power, 2016-09-09 20:16:28]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/infra/infra/+/27d3c91752defc8bf2556775462f4...