Add ratio calculation to take the ratio of two calculations.

Add ratio function to calculate the ratio of two calculations.

To test this:

$ cd ~/golib/src/
$ go get -u -t skia.googlesource.com/buildbot.git/perf/go/...
$ cd skia.googlesource.com/buildbot.git/perf/
$ make testgo

BUG=None
TEST=see above
R=jcgregorio@google.com

Committed: https://skia.googlesource.com/buildbot/+/27b13788268f622c7936e0e9591e09aa188a8e19

[jcgregorio, 2014-09-29 15:09:23]

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:186: traces, err := node.Args[0].Eval(ctx)
Yes, you'll need to call Eval on both Args[0] and Args[1].

[tfarina, 2014-09-30 13:35:15]

more doubts below.

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:195: ret := types.NewPerfTraceN(len(traces[0].Values))
Where the calc will go here?

You said in the e-mail the calculation could be:
a.Values[i] / b.Values[i]

Does that translates to the following?

traces[0].Values[i] / traces[1].Values[i]

This 'i' will come from a for loop?

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:200: return `ratio() .`
could the description be something like:

"ratio() returns the ratio of two calculations."

Suggest something better?

[jcgregorio, 2014-09-30 14:59:03]

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:195: ret := types.NewPerfTraceN(len(traces[0].Values))
On 2014/09/30 13:35:14, tfarina wrote:
> Where the calc will go here?
> 
> You said in the e-mail the calculation could be:
> a.Values[i] / b.Values[i]
> 
> Does that translates to the following?
> 
> traces[0].Values[i] / traces[1].Values[i]
> 
> This 'i' will come from a for loop?

Yes, look at

  for i, _ := range ret.Values

in AveFunc.Eval.

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:200: return `ratio() .`
On 2014/09/30 13:35:14, tfarina wrote:
> could the description be something like:
> 
> "ratio() returns the ratio of two calculations."
> 
> Suggest something better?

`ratio(a, b) returns the point by point ratio of two traces.

That is it returns a trace with a[i]/b[i] for every point in a and b.`

[tfarina, 2014-10-01 02:30:16]

More progress (but little).

Not sure yet how to develop the for loop for the traces. 

Waiting for your feedback.

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:186: traces, err := node.Args[0].Eval(ctx)
On 2014/09/29 15:09:22, jcgregorio wrote:
> Yes, you'll need to call Eval on both Args[0] and Args[1].

Done.

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:195: ret := types.NewPerfTraceN(len(traces[0].Values))
On 2014/09/30 14:59:03, jcgregorio wrote:
> On 2014/09/30 13:35:14, tfarina wrote:
> > Where the calc will go here?
> > 
> > You said in the e-mail the calculation could be:
> > a.Values[i] / b.Values[i]
> > 
> > Does that translates to the following?
> > 
> > traces[0].Values[i] / traces[1].Values[i]
> > 
> > This 'i' will come from a for loop?
> 
> Yes, look at
> 
>   for i, _ := range ret.Values
> 
I'm not sure yet how to proceed. Now I have tracesA and tracesB.

Will they have the same length?

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:200: return `ratio() .`
On 2014/09/30 14:59:03, jcgregorio wrote:
> On 2014/09/30 13:35:14, tfarina wrote:
> > could the description be something like:
> > 
> > "ratio() returns the ratio of two calculations."
> > 
> > Suggest something better?
> 
> `ratio(a, b) returns the point by point ratio of two traces.
> 
> That is it returns a trace with a[i]/b[i] for every point in a and b.`

Done.

[jcgregorio, 2014-10-01 03:48:58]

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/20001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:195: ret := types.NewPerfTraceN(len(traces[0].Values))
Yes, they will always have the same length.

On 2014/10/01 02:30:16, tfarina wrote:
> On 2014/09/30 14:59:03, jcgregorio wrote:
> > On 2014/09/30 13:35:14, tfarina wrote:
> > > Where the calc will go here?
> > > 
> > > You said in the e-mail the calculation could be:
> > > a.Values[i] / b.Values[i]
> > > 
> > > Does that translates to the following?
> > > 
> > > traces[0].Values[i] / traces[1].Values[i]
> > > 
> > > This 'i' will come from a for loop?
> > 
> > Yes, look at
> > 
> >   for i, _ := range ret.Values
> > 
> I'm not sure yet how to proceed. Now I have tracesA and tracesB.
> 
> Will they have the same length?

[tfarina, 2014-10-16 17:42:31]

I will get back to this later tonight.

Some notes for myself below.

https://codereview.chromium.org/610793003/diff/80001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/80001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:198: fmt.Println(traces[0].Values[i])
ret.Values[i] = traces[0].Values[i] / traces[1].Values[i]

https://codereview.chromium.org/610793003/diff/80001/perf/go/parser/funcs.go#...
perf/go/parser/funcs.go:201: return nil, nil
return []*types.PerfTrace{ret}, nil

https://codereview.chromium.org/610793003/diff/80001/perf/go/parser/parser_te...
File perf/go/parser/parser_test.go (right):

https://codereview.chromium.org/610793003/diff/80001/perf/go/parser/parser_te...
perf/go/parser/parser_test.go:136:
ctx.Tile.Traces["t1"].(*types.PerfTrace).Values = []float64{1.0, -1.0, 1e100,
1e100}
I think I can have more reasonable numbers in these slices. For easier
computation.

https://codereview.chromium.org/610793003/diff/80001/perf/go/parser/parser_te...
perf/go/parser/parser_test.go:138: }
Then have:

traces, err := ctx.Eval(`ratio( 
ave(fill(filter("source_type=skp&config=gpu&bench_type=playback"))),
ave(fill(filter("source_type=skp&config=8888&bench_type=playback")))
)`)

[tfarina, 2014-10-17 00:35:47]

Joe, could you take another look?

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:191: traces, err = node.Args[1].Eval(ctx)
why I'm getting zero traces here? len(traces) == 0

[jcgregorio, 2014-10-17 11:43:55]

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:186: traces, err := node.Args[0].Eval(ctx)
tracesA, err :=

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:191: traces, err = node.Args[1].Eval(ctx)
tracesB, err :=

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:191: traces, err = node.Args[1].Eval(ctx)
On 2014/10/17 00:35:47, tfarina wrote:
> why I'm getting zero traces here? len(traces) == 0

The filtering in the test is too narrow and coming up with 0 traces that match.
See my comments in the test file.

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:196: ret := types.NewPerfTraceN(len(traces[0].Values))
ret := types.NewPerfTraceN(len(tracesA[0].Values))

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:198: ret.Values[i] = traces[0].Values[i] /
traces[1].Values[i]
ret.Values[i] = tracesA[0].Values[i] / tracesB[1].Values[i]

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/parser_t...
File perf/go/parser/parser_test.go (right):

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/parser_t...
perf/go/parser/parser_test.go:140:
ave(fill(filter("source_type=skp&config=gpu&bench_type=playback"))),
ave(fill(filter("config=gpu"))),

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/parser_t...
perf/go/parser/parser_test.go:141:
ave(fill(filter("source_type=skp&config=8888&bench_type=playback")))
ave(fill(filter("config=8888"))),

[tfarina, 2014-10-17 21:14:23]

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:186: traces, err := node.Args[0].Eval(ctx)
On 2014/10/17 11:43:55, jcgregorio wrote:
> tracesA, err :=

Done.

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:186: traces, err := node.Args[0].Eval(ctx)
On 2014/10/17 11:43:55, jcgregorio wrote:
> tracesA, err :=

Done.

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:191: traces, err = node.Args[1].Eval(ctx)
On 2014/10/17 11:43:55, jcgregorio wrote:
> tracesB, err :=

Done.

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:196: ret := types.NewPerfTraceN(len(traces[0].Values))
On 2014/10/17 11:43:55, jcgregorio wrote:
> ret := types.NewPerfTraceN(len(tracesA[0].Values))

Done.

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:198: ret.Values[i] = traces[0].Values[i] /
traces[1].Values[i]
On 2014/10/17 11:43:55, jcgregorio wrote:
> ret.Values[i] = tracesA[0].Values[i] / tracesB[1].Values[i]

Done.

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/parser_t...
File perf/go/parser/parser_test.go (right):

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/parser_t...
perf/go/parser/parser_test.go:140:
ave(fill(filter("source_type=skp&config=gpu&bench_type=playback"))),
On 2014/10/17 11:43:55, jcgregorio wrote:
> ave(fill(filter("config=gpu"))),

Done.

https://codereview.chromium.org/610793003/diff/100001/perf/go/parser/parser_t...
perf/go/parser/parser_test.go:141:
ave(fill(filter("source_type=skp&config=8888&bench_type=playback")))
On 2014/10/17 11:43:55, jcgregorio wrote:
> ave(fill(filter("config=8888"))),

Done.

[jcgregorio, 2014-10-18 00:19:13]

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:198: ret.Values[i] = tracesA[0].Values[i] /
tracesB[1].Values[i]
Either check for division by 0 before hand, or check the result of the division
for +-Inf after the fact and replace them with MISSING_DATA_SENTINEL if found.

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
File perf/go/parser/parser_test.go (right):

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
perf/go/parser/parser_test.go:137:
ctx.Tile.Traces["t2"].(*types.PerfTrace).Values = []float64{5, 2, 4, 5}
Add a divide by zero case to the tests.

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
perf/go/parser/parser_test.go:144: }
Add tests for the values coming from the result of the Eval.

[tfarina, 2014-10-18 02:58:06]

check if the result of division is infinity - that can happen when you divide by
zero

[tfarina, 2014-10-18 03:15:46]

Joe, I think I addressed everything. 'make testgo' is passing now.

I think it is ready for another review round.

Thank you for all the guidance ;)

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/funcs.go
File perf/go/parser/funcs.go (right):

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/funcs.go...
perf/go/parser/funcs.go:198: ret.Values[i] = tracesA[0].Values[i] /
tracesB[1].Values[i]
On 2014/10/18 00:19:13, jcgregorio wrote:
> Either check for division by 0 before hand, or check the result of the
division
> for +-Inf after the fact and replace them with MISSING_DATA_SENTINEL if found.

Done.

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
File perf/go/parser/parser_test.go (right):

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
perf/go/parser/parser_test.go:137:
ctx.Tile.Traces["t2"].(*types.PerfTrace).Values = []float64{5, 2, 4, 5}
On 2014/10/18 00:19:13, jcgregorio wrote:
> Add a divide by zero case to the tests.

Done.

https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
perf/go/parser/parser_test.go:144: }
On 2014/10/18 00:19:13, jcgregorio wrote:
> Add tests for the values coming from the result of the Eval.

Done.

[jcgregorio, 2014-10-20 14:16:05]

On 2014/10/18 03:15:46, tfarina wrote:
> Joe, I think I addressed everything. 'make testgo' is passing now.
> 
> I think it is ready for another review round.
> 
> Thank you for all the guidance ;)
> 
> https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/funcs.go
> File perf/go/parser/funcs.go (right):
> 
>
https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/funcs.go...
> perf/go/parser/funcs.go:198: ret.Values[i] = tracesA[0].Values[i] /
> tracesB[1].Values[i]
> On 2014/10/18 00:19:13, jcgregorio wrote:
> > Either check for division by 0 before hand, or check the result of the
> division
> > for +-Inf after the fact and replace them with MISSING_DATA_SENTINEL if
found.
> 
> Done.
> 
>
https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
> File perf/go/parser/parser_test.go (right):
> 
>
https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
> perf/go/parser/parser_test.go:137:
> ctx.Tile.Traces["t2"].(*types.PerfTrace).Values = []float64{5, 2, 4, 5}
> On 2014/10/18 00:19:13, jcgregorio wrote:
> > Add a divide by zero case to the tests.
> 
> Done.
> 
>
https://codereview.chromium.org/610793003/diff/120001/perf/go/parser/parser_t...
> perf/go/parser/parser_test.go:144: }
> On 2014/10/18 00:19:13, jcgregorio wrote:
> > Add tests for the values coming from the result of the Eval.
> 
> Done.

LGTM

[jcgregorio, 2014-10-20 14:35:35]

The CQ bit was checked by jcgregorio@google.com

[commit-bot: I haz the power, 2014-10-20 14:36:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610793003/220001

[commit-bot: I haz the power, 2014-10-20 14:36:37]

Committed patchset #12 (id:220001) as 27b13788268f622c7936e0e9591e09aa188a8e19