Closed
Bug 849844
Opened 11 years ago
Closed 11 years ago
add errorbars to the phonedash graph.
Categories
(Testing Graveyard :: Autophone, defect)
Testing Graveyard
Autophone
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bc, Assigned: bc)
Details
Attachments
(2 files)
20.99 KB,
patch
|
mcote
:
review+
|
Details | Diff | Splinter Review |
21.39 KB,
patch
|
mcote
:
feedback+
|
Details | Diff | Splinter 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 1•11 years ago
|
||
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+
Assignee | ||
Comment 2•11 years ago
|
||
(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.
Comment 3•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
I think this covers all the points.
Attachment #723751 -
Flags: feedback?(mcote)
Comment 5•11 years ago
|
||
Comment on attachment 723751 [details] [diff] [review] patch v2 Review of attachment 723751 [details] [diff] [review]: ----------------------------------------------------------------- Great!
Attachment #723751 -
Flags: feedback?(mcote) → feedback+
Assignee | ||
Comment 6•11 years ago
|
||
https://github.com/markrcote/phonedash/commit/15432635e2a3318f32858f61f07d380d9f7bc365
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•2 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•