Refactor significance calculations #1003
Conversation
This moves significance factor calculation to the backend, and refactors the code surrounding that, but makes no changes yet.
| .unwrap_or(Self::SIGNIFICANT_RELATIVE_CHANGE_THRESHOLD) | ||
| } | ||
| } | ||
|
|
||
| /// This is a numeric magnitude of a particular change. | ||
| fn significance_factor(&self) -> Option<f64> { |
There was a problem hiding this comment.
What's the reason for this being an optional?
There was a problem hiding this comment.
Hm, I think this was leftover from an earlier version I had locally. The reasoning was that we may not always be able to compute a change's magnitude in a reasonable way -- we approximate significance with the 0.002 threshold constants, but there's no such thing really for "how significant" if there's no prior data.
I actually think this may be a little buggy in the sense that I think we should show something like "-" in the UI if we don't have enough historical data to judge the factor, not just scale based on the 0.002 threshold, which would be misleading.
| fn significance_threshold(&self) -> f64 { | ||
| let (q1, median, q3) = self.quartiles(); | ||
|
|
||
| // Changes that are IQR_MULTIPLIER away from the median are considered |
There was a problem hiding this comment.
Can you talk more about why you changed calculation from q3 + IQR to median + IQR? I believe outlier fences are normally calculated from the quartiles and not from the median.
There was a problem hiding this comment.
Hm. So my reasoning here was that it felt like using the median is somehow "better" -- adding IQR to the median seems more reasonable than adding it to Q3, which sort of already includes part of the IQR. This was also partially based on my (incorrect) recollection was that my statistics classes did have it based on the median, though since the pages you've linked to do all have q3, I was probably just wrong here :)
In practice our median and q3 are usually pretty close, so this probably has a negligible difference.
There was a problem hiding this comment.
Hmm I'm not sure if it feels better to me. Perhaps you could try to explain more why it does?
In any case if it's unlikely to make a huge difference, perhaps we should just stick with the text book definition?
There was a problem hiding this comment.
I'm happy to revert this particular change.
I think it mostly felt like we ended up with something like
median + IQR * (some coefficient representing how "right skewed" we are) + IQR * IQR_MULTIPLIER
where the middle term is the Q3-median delta. That felt a little weird for our purposes, where we don't actually care about the right-skew that much, in practice we will always have high right skew since all data is going to be bound to [0, infinity) so there's practically no way to be left skewed.
There was a problem hiding this comment.
That makes sense. I don't feel too strongly about this which I guess is why my gut says to fallback to the book mechanism, but I'll leave it to you.
However, can you update the docs where we explain this? https://github.com/rust-lang/rustc-perf/blob/master/docs/comparison-analysis.md#what-makes-a-test-result-significant
There was a problem hiding this comment.
I'll just revert this change I think, that seems best. I'll update the constant 1.5 in the docs as well.
This moves significance factor calculation to the backend and does some refactoring around that, but doesn't actually change the calculation significantly.
The minor difference is that the significance threshold is calculated as
median + ...rather thanq3 + ..., which seems more appropriate. Basing the significance threshold at the q3 marker introduces a sort of "partial iqr" and that feels fishy.