Changes bench_compare to be able to specify representation algorithms with flags.

Changes bench_compare to be able to specify representation algorithms with flags.

[epoger, 2012-08-17 14:09:08]

I don't understand this change.

Shouldn't bench_compare be able to tell what representation algorithm was used
to generate the bench output, just by looking at it?

If input file 1 and input file 2 used DIFFERENT algorithms, how can they be
compared anyway?

I think we would really benefit from 10 minutes of live conversation about it...

[TomH, 2012-08-17 14:23:28]

Reading over bench_utils::parse(), the bench output has all the data necessary
for any of the algorithms to run, and these flags are telling it *how* we want
bench_compare to do the comparison?

So maybe the issue is just naming/documentation rather than something
fundamental.

[epoger, 2012-08-17 14:37:48]

On 2012/08/17 14:23:28, TomH wrote:
> Reading over bench_utils::parse(), the bench output has all the data necessary
> for any of the algorithms to run, and these flags are telling it *how* we want
> bench_compare to do the comparison?
> 
> So maybe the issue is just naming/documentation rather than something
> fundamental.

If we're telling bench_compare HOW to do the comparison, we only need one flag,
not "representation algorithm for old bench" and "representation algorithm for
new bench".

[benchen, 2012-08-17 15:08:49]

I thought about it, and decided to use two flags, so people can set the old and
new files to be the same file and use different algorithms on it. It shows which
benches differ the most/least with various algorithms. Does it make sense?

How about adding this to the top description:

"Flags -a and -m set the representation algorithm (see bench_util) used for old
and new benches. You can set them differently to evaluate the diff of algorithms
on the same bench file (-o and -n)."

On 2012/08/17 14:37:48, epoger wrote:
> On 2012/08/17 14:23:28, TomH wrote:
> > Reading over bench_utils::parse(), the bench output has all the data
necessary
> > for any of the algorithms to run, and these flags are telling it *how* we
want
> > bench_compare to do the comparison?
> > 
> > So maybe the issue is just naming/documentation rather than something
> > fundamental.
> 
> If we're telling bench_compare HOW to do the comparison, we only need one
flag,
> not "representation algorithm for old bench" and "representation algorithm for
> new bench".

[TomH, 2012-08-17 15:10:58]

Let's at least have a default/standard way to specify once and have that used
for both.

The proposed text is really unclear. In part because "representation algorithm"
is not a good description of what we're doing.

[benchen, 2012-08-17 15:14:38]

Sorry for my bad English. Could you please help me come up with a clearer
description?
How about using -s to set algorithm for both?

On 2012/08/17 15:10:58, TomH wrote:
> Let's at least have a default/standard way to specify once and have that used
> for both.
> 
> The proposed text is really unclear. In part because "representation
algorithm"
> is not a good description of what we're doing.

[bungeman, 2012-08-17 15:20:56]

The way I would structure this is to have 'avg' be the default default, a '-a'
parameter to specify the default algorithm, and longer named parameters like
'--oa' and '--na' for specifying the algorithm for the old file and new file
respectively. See bench_graph_svg.py's main 'default-setting' for an existing
long name parameter example.

[benchen, 2012-08-17 15:24:24]

Thank you Ben. I did try to use oa and na but failed to find that example.
Will make the changes and waiting for Tom's draft comments.

On 2012/08/17 15:20:56, bungeman wrote:
> The way I would structure this is to have 'avg' be the default default, a '-a'
> parameter to specify the default algorithm, and longer named parameters like
> '--oa' and '--na' for specifying the algorithm for the old file and new file
> respectively. See bench_graph_svg.py's main 'default-setting' for an existing
> long name parameter example.

[TomH, 2012-08-17 15:25:16]

Or something we do in other tools is
 - have a default default
 - if one value is specified, apply it to both

So I'd say
"-a (avg|min|med) determines whether the average, minimum, or median of each
benchmark is used in the comparison.
[-m (avg|min|med)] causes a different analysis to be applied to the new (-n)
input file than -a specified for the old (-o) file."

Bikeshed: hmm, does bench_compare require a flag for every argument? Ugh.

[benchen, 2012-08-17 15:27:46]

The default default is set in _ListAlgorithm in bench_util. Do I have to say
that the defaults are set there??

On 2012/08/17 15:25:16, TomH wrote:
> Or something we do in other tools is
>  - have a default default
>  - if one value is specified, apply it to both
> 
> So I'd say
> "-a (avg|min|med) determines whether the average, minimum, or median of each
> benchmark is used in the comparison.
> [-m (avg|min|med)] causes a different analysis to be applied to the new (-n)
> input file than -a specified for the old (-o) file."
> 
> Bikeshed: hmm, does bench_compare require a flag for every argument? Ugh.

[benchen, 2012-08-17 15:29:48]

Tom's suggestion on flags requires one fewer flag, so will go with that.

On 2012/08/17 15:27:46, benchen wrote:
> The default default is set in _ListAlgorithm in bench_util. Do I have to say
> that the defaults are set there??
> 
> On 2012/08/17 15:25:16, TomH wrote:
> > Or something we do in other tools is
> >  - have a default default
> >  - if one value is specified, apply it to both
> > 
> > So I'd say
> > "-a (avg|min|med) determines whether the average, minimum, or median of each
> > benchmark is used in the comparison.
> > [-m (avg|min|med)] causes a different analysis to be applied to the new (-n)
> > input file than -a specified for the old (-o) file."
> > 
> > Bikeshed: hmm, does bench_compare require a flag for every argument? Ugh.

[TomH, 2012-08-17 15:34:05]

Oh, important note on convention (yes, we have *one* tool that breaks this, but
everything else follows it):

- for an abbreviated flag
-- for a spelled-out flag name

[benchen, 2012-08-17 16:03:08]

Thanks Tom. I changed flag behavior as suggested. Let me know how I should
reword the flag descriptions.

[TomH, 2012-08-17 20:57:04]

On 2012/08/17 16:03:08, benchen wrote:
> Thanks Tom. I changed flag behavior as suggested. Let me know how I should
> reword the flag descriptions.

In comment #10 I suggested:

So I'd say
"-a (avg|min|med) determines whether the average, minimum, or median of each
benchmark is used in the comparison.
[-m (avg|min|med)] causes a different analysis to be applied to the new (-n)
input file than -a specified for the old (-o) file."

But that's just a strawman.

[benchen, 2012-08-17 21:54:33]

Blame my blindness for this one. Sorry and thanks Tom. Now it's clear and
concise. Please take another look.

On 2012/08/17 20:57:04, TomH wrote:
> On 2012/08/17 16:03:08, benchen wrote:
> > Thanks Tom. I changed flag behavior as suggested. Let me know how I should
> > reword the flag descriptions.
> 
> In comment #10 I suggested:
> 
> So I'd say
> "-a (avg|min|med) determines whether the average, minimum, or median of each
> benchmark is used in the comparison.
> [-m (avg|min|med)] causes a different analysis to be applied to the new (-n)
> input file than -a specified for the old (-o) file."
> 
> But that's just a strawman.

[benchen, 2012-08-20 19:32:49]

Ping on this issue. Let me know if/when I can commit. Thanks.

On 2012/08/17 21:54:33, benchen wrote:
> Blame my blindness for this one. Sorry and thanks Tom. Now it's clear and
> concise. Please take another look.
> 
> On 2012/08/17 20:57:04, TomH wrote:
> > On 2012/08/17 16:03:08, benchen wrote:
> > > Thanks Tom. I changed flag behavior as suggested. Let me know how I should
> > > reword the flag descriptions.
> > 
> > In comment #10 I suggested:
> > 
> > So I'd say
> > "-a (avg|min|med) determines whether the average, minimum, or median of each
> > benchmark is used in the comparison.
> > [-m (avg|min|med)] causes a different analysis to be applied to the new (-n)
> > input file than -a specified for the old (-o) file."
> > 
> > But that's just a strawman.

[epoger, 2012-08-30 17:19:49]

On 2012/08/20 19:32:49, benchen wrote:
> Ping on this issue. Let me know if/when I can commit. Thanks.

Ben- In http://codereview.appspot.com/6461110/ ('Update bench_compare to do the
comparison using different statistical methods'), you wrote that you would close
this issue.  Right?

[benchen, 2012-08-30 18:11:47]

Yes, the status has been "closed" since 10 days ago, but I forgot to put a note
here. Thanks!

On 2012/08/30 17:19:49, epoger wrote:
> On 2012/08/20 19:32:49, benchen wrote:
> > Ping on this issue. Let me know if/when I can commit. Thanks.
> 
> Ben- In http://codereview.appspot.com/6461110/ ('Update bench_compare to do
the
> comparison using different statistical methods'), you wrote that you would
close
> this issue.  Right?