Closed Bug 849844 Opened 7 years ago Closed 7 years ago

add errorbars to the phonedash graph.

Categories

(Testing :: Autophone, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bc, Assigned: bc)

Details

Attachments

(2 files)

Attached patch patchSplinter Review
This patch the ability to:

1. calculate the std deviation of the data reported to phonedash.
2. optionally display the std deviation on the phonedash graph.
3. optionally only display the first data point.

I'll send you some good example data to host at mcote_bc.

This works well with one exception. When I update the browser and restart when I have the graph displayed with the error bars checked, the page will get into an infinite loop where it continually checks and unchecks the errorbars. This does not happen when starting the browser with a saved session though.

I'm pretty weak on the way forms are handled in phonedash and there is probably something silly I'm doing wrong.
Attachment #723473 - Flags: review?(mcote)
Comment on attachment 723473 [details] [diff] [review]
patch

Review of attachment 723473 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few nits.

::: html/index.html
@@ +46,5 @@
>          </select>
> +        <br />
> +        Show error bars <input name="errorbars" id="errorbars" type="checkbox"  />
> +        <br />
> +        Show Initial only <input name="initialonly" id="initialonly" type="checkbox" />

Don't capitalize "Initial".

::: html/scripts/app.js
@@ +142,5 @@
>    var hash = '#/' + params.product + '/' + params.metric + '/' + params.test +
>          '/' + $('#startdate').attr('value') +
> +        '/' + $('#enddate').attr('value') +
> +        '/' + $('#errorbars').attr('checked') +
> +        '/' + $('#initialonly').attr('checked');

Would be kind of nice if this didn't result in urls ending in "/undefined".  Makes it looks like there was an error or something.  Maybe $('#errorbars').attr('checked') ? 'checked' : 'off' or something?

@@ +147,5 @@
>    if (hash != document.location.hash) {
>      document.location.hash = hash;
>      return false;
>    }
> +  $.getJSON('api/s1s2/data/?product=' + params.product + '&metric=' + params.metric + '&test=' + params.test + '&start=' + $('#startdate').attr('value') + '&end=' + $('#enddate').attr('value') + '&errorbars=' + $('#errorbars').attr('checked') + '&initialonly=' + $('#initialonly').attr('checked'), function(data) {

Why do we submit the value of #errorbars?  Doesn't look like the API does anything with it...

::: server/handlers.py
@@ +127,4 @@
>                  del r['values']
>          return results
> +
> +def getMeanStdDev(values):

Standard Python coding style (PEP-8) specifies underscores instead of mixed case for function names.

@@ +127,5 @@
>                  del r['values']
>          return results
> +
> +def getMeanStdDev(values):
> +    from math import sqrt

Move to top so we don't do the import on every call to this function.

@@ +129,5 @@
> +
> +def getMeanStdDev(values):
> +    from math import sqrt
> +    count = len(values)
> +    if count < 2:

I think this would be clearer as count == 1, just because the following line presumes that there is at least one element in values.
Attachment #723473 - Flags: review?(mcote) → review+
(In reply to Mark Côté ( :mcote ) from comment #1)

> 
> Would be kind of nice if this didn't result in urls ending in "/undefined". 
> Makes it looks like there was an error or something.  Maybe
> $('#errorbars').attr('checked') ? 'checked' : 'off' or something?

Yeah, I had some difficulty with this. I'll try to make it nicer.

> 
> 
> Why do we submit the value of #errorbars?  Doesn't look like the API does
> anything with it...

I didn't at first, but when I needed to pass the initialonly to the handler, I realized I had left out the ability for the handler to get it. I did it mostly for consistency. We *could* ignore the std dev calculation if we didn't have errorbars checked....

I can take it out if you want.

> 
> ::: server/handlers.py
> @@ +127,4 @@
> >                  del r['values']
> >          return results
> > +
> > +def getMeanStdDev(values):
> 
> Standard Python coding style (PEP-8) specifies underscores instead of mixed
> case for function names.

Oh. I was trying to be consistent with the file, but those are all ClassNames! ok.

> 
> @@ +127,5 @@
> >                  del r['values']
> >          return results
> > +
> > +def getMeanStdDev(values):
> > +    from math import sqrt
> 
> Move to top so we don't do the import on every call to this function.
> 

Ok.

> @@ +129,5 @@
> > +
> > +def getMeanStdDev(values):
> > +    from math import sqrt
> > +    count = len(values)
> > +    if count < 2:
> 
> I think this would be clearer as count == 1, just because the following line
> presumes that there is at least one element in values.

Ok.
(In reply to Bob Clary [:bc:] from comment #2)
> (In reply to Mark Côté ( :mcote ) from comment #1)
> > Why do we submit the value of #errorbars?  Doesn't look like the API does
> > anything with it...
> 
> I didn't at first, but when I needed to pass the initialonly to the handler,
> I realized I had left out the ability for the handler to get it. I did it
> mostly for consistency. We *could* ignore the std dev calculation if we
> didn't have errorbars checked....
> 
> I can take it out if you want.

It's fine for now, until we rework the UI issues anyway.
Attached patch patch v2Splinter Review
I think this covers all the points.
Attachment #723751 - Flags: feedback?(mcote)
Comment on attachment 723751 [details] [diff] [review]
patch v2

Review of attachment 723751 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #723751 - Flags: feedback?(mcote) → feedback+
https://github.com/markrcote/phonedash/commit/15432635e2a3318f32858f61f07d380d9f7bc365
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.