Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
241273002:
Change "percentage change" function and add comments/test for it. (Closed)
|
Created:
6 years, 8 months ago by qyearsley Modified:
6 years, 8 months ago CC:
chromium-reviews, tonyg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionChange "percentage change" function and add comments/test for it.
Goal: To make it clearer how percentage change is calculated, and make it so that it's the same as how the dashboard calculates it.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265791
Patch Set 1 #
Total comments: 6
Patch Set 2 : Merge latest changes if any #
Total comments: 6
Patch Set 3 : Respond to comments #Patch Set 4 : Adjust test #Patch Set 5 : #Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression... tools/bisect-perf-regression.py:190: """Calculates the truncated mean of a set of values. I just did a quick search, and it appears that calculation of "truncated mean" (or "trimmed mean") usually doesn't involve; usually the X% trimmed mean is calculated by just excluding the top X% and bottom X% and taking the mean of the rest of the values. Reference: http://www.d.umn.edu/~yqi/stat3611/trimmed.pdf http://voices.yahoo.com/statistics-101-trimmed-mean-median-4152059.html http://www.statistics.com/glossary&term_id=866 However, calculation of the "interquartile mean" may involve weighting two of the values. http://en.wikipedia.org/wiki/Interquartile_mean Any thoughts? I'm thinking of replacing this with the simplified truncated mean function, which doesn't weight anything. https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression... tools/bisect-perf-regression.py:298: two numbers; sometimes it is defined as relative to the smaller number, The previous version was calculating the difference relative to the smaller number. It was calculating: (big/small) - 1.0 = (big/small) - (small/small) = (big - small) / small
https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression... tools/bisect-perf-regression.py:190: """Calculates the truncated mean of a set of values. I don't have a strong opinion on it, so go ahead with whatever you feel is best. http://en.wikipedia.org/wiki/Truncated_mean This does define the truncated/trimmed mean as using weighted values. Additionally, according to the wikipedia entry you linked, the interquartile mean is just the truncated mean with specific values (in this case, 25%). On 2014/04/17 18:47:59, qyearsley wrote: > I just did a quick search, and it appears that calculation of "truncated mean" > (or "trimmed mean") usually doesn't involve; usually the X% trimmed mean is > calculated by just excluding the top X% and bottom X% and taking the mean of the > rest of the values. > > Reference: > http://www.d.umn.edu/~yqi/stat3611/trimmed.pdf > http://voices.yahoo.com/statistics-101-trimmed-mean-median-4152059.html > http://www.statistics.com/glossary&term_id=866 > > However, calculation of the "interquartile mean" may involve weighting two of > the values. > http://en.wikipedia.org/wiki/Interquartile_mean > > Any thoughts? I'm thinking of replacing this with the simplified truncated mean > function, which doesn't weight anything. https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression... tools/bisect-perf-regression.py:298: two numbers; sometimes it is defined as relative to the smaller number, Does this change make it match the perf dashboard? On 2014/04/17 18:47:59, qyearsley wrote: > The previous version was calculating the difference relative to the smaller > number. > > It was calculating: > (big/small) - 1.0 > = (big/small) - (small/small) > = (big - small) / small https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:309: if before == 0: If the "good" values were legitimately 0 (ie. the values being bisected were # of memory leaks or some such), won't this report a regression size of nan? Don't think it functionally changes anything, just might look a bit funny. https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:311: difference = math.fabs(after - before) This fabs call seems unnecessary, considering you do it again on the next line.
https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression... tools/bisect-perf-regression.py:298: two numbers; sometimes it is defined as relative to the smaller number, Doh! This is answered in your CL description :) On 2014/04/17 19:46:17, shatch wrote: > Does this change make it match the perf dashboard? > > On 2014/04/17 18:47:59, qyearsley wrote: > > The previous version was calculating the difference relative to the smaller > > number. > > > > It was calculating: > > (big/small) - 1.0 > > = (big/small) - (small/small) > > = (big - small) / small >
https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/241273002/diff/1/tools/bisect-perf-regression... tools/bisect-perf-regression.py:190: """Calculates the truncated mean of a set of values. Ah, right! I overlooked that. I think that the current truncated mean function is fine, I just had a bit of trouble understanding it. https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:309: if before == 0: On 2014/04/17 19:46:17, shatch wrote: > If the "good" values were legitimately 0 (ie. the values being bisected were # > of memory leaks or some such), won't this report a regression size of nan? Don't > think it functionally changes anything, just might look a bit funny. Yeah -- In this version before my latest patch, the output would be: Bisect reproduced a nan (+-stderr%) change ... But it might be more informative to output: Bisect reproduced a zero-to-nonzero (+-stderr%) change... Note that the dashboard has a special case from zero-to-nonzero; Annie used the string "freakin' huge", so it says "Found a freakin' huge regression in ...". This occasionally confuses some sheriffs. In the latest patch set for this CL, I've made a change so that if this function returns NaN, then the message printed should be "zero-to-nonzero". If you think it's better, We could also return some other value like 10000000 or "zero to nonzero". https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:311: difference = math.fabs(after - before) On 2014/04/17 19:46:17, shatch wrote: > This fabs call seems unnecessary, considering you do it again on the next line. Good point, done. (My original thinking was: first line get the absolute difference. Second line also get abs value because |before| might be negative. But the result is the same if abs is not taken of the difference.)
lgtm, please wait for Prasad to take a look. https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:309: if before == 0: Hmm, yeah I'm not sure what the best output here would be. "zero-to-nonzero" doesn't feel right either, definitely better than nan though. Maybe Prasad has some thoughts on this. On 2014/04/17 20:54:05, qyearsley wrote: > On 2014/04/17 19:46:17, shatch wrote: > > If the "good" values were legitimately 0 (ie. the values being bisected were # > > of memory leaks or some such), won't this report a regression size of nan? > Don't > > think it functionally changes anything, just might look a bit funny. > > Yeah -- In this version before my latest patch, the output would be: > Bisect reproduced a nan (+-stderr%) change ... > > But it might be more informative to output: > Bisect reproduced a zero-to-nonzero (+-stderr%) change... > > Note that the dashboard has a special case from zero-to-nonzero; Annie used the > string "freakin' huge", so it says "Found a freakin' huge regression in ...". > This occasionally confuses some sheriffs. > > In the latest patch set for this CL, I've made a change so that if this function > returns NaN, then the message printed should be "zero-to-nonzero". If you think > it's better, We could also return some other value like 10000000 or "zero to > nonzero".
https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:309: if before == 0: Anyway, if we're calculating relative change relative to 0, I don't think a numerical return value is very helpful. The other way of calculating relative change has the same issue; the relative change from 1 to 0 or 0 to 1 would have been: (1 / 0.0001) * 100 - 100 = 999900.0
On 2014/04/18 19:38:03, qyearsley wrote: > https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... > File tools/bisect-perf-regression.py (right): > > https://codereview.chromium.org/241273002/diff/20001/tools/bisect-perf-regres... > tools/bisect-perf-regression.py:309: if before == 0: > Anyway, if we're calculating relative change relative to 0, I don't think a > numerical return value is very helpful. > > The other way of calculating relative change has the same issue; the relative > change from 1 to 0 or 0 to 1 would have been: > (1 / 0.0001) * 100 - 100 > = 999900.0 Sorry, let this slip. Yeah, agree that numerical doesn't make any sense. Don't have anything better than "zero-to-nonzero" so still lgtm.
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qyearsley@chromium.org/241273002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qyearsley@chromium.org/241273002/80001
Message was sent while issue was closed.
Change committed as 265791 |
