Fix Hodges-Lehmann distribution ratio calculation#111
Fix Hodges-Lehmann distribution ratio calculation#111mgrzaslewicz wants to merge 2 commits intomasterfrom
Conversation
mgrzaslewicz
force-pushed
the
fix-hodges-lehmann
branch
from
ceafd55 to
fee23ec
Compare
mgrzaslewicz
force-pushed
the
fix-hodges-lehmann
branch
from
fee23ec to
1d2eb85
Compare
src/test/kotlin/com/atlassian/performance/tools/report/ShiftedDistributionRegressionTestTest.kt
Show resolved
Hide resolved
...ain/kotlin/com/atlassian/performance/tools/report/api/distribution/DistributionComparator.kt
Show resolved
Hide resolved
...ain/kotlin/com/atlassian/performance/tools/report/api/distribution/DistributionComparator.kt
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/atlassian/performance/tools/report/api/distribution/DistributionComparator.kt
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/atlassian/performance/tools/report/api/distribution/DistributionComparator.kt
Outdated
Show resolved
Hide resolved
| import com.atlassian.performance.tools.jiraactions.api.ActionType | ||
| import com.atlassian.performance.tools.report.ActionMetricsReader | ||
| import com.atlassian.performance.tools.report.api.ShiftedDistributionRegressionTest | ||
| import com.atlassian.performance.tools.report.api.distribution.DistributionComparator |
There was a problem hiding this comment.
No difference detected by RelativeNonparametricPerformanceJudgeTest?
There was a problem hiding this comment.
So we don't see why the new one is better.
There was a problem hiding this comment.
How do you define better? Classification has not changed for unit tested cases and as stated in first PR comment, it's expected
src/main/kotlin/com/atlassian/performance/tools/report/api/ShiftedDistributionRegressionTest.kt
Outdated
Show resolved
Hide resolved
| values[k++] = func(baseline[i], experiment[j]) | ||
| } | ||
| } | ||
| return Median().withNaNStrategy(NaNStrategy.MINIMAL).evaluate(values) |
There was a problem hiding this comment.
When used on a set of latency measurements, why would we inject fake negative infinities?
There was a problem hiding this comment.
It's needed for cases when part of the distribution is the same and other part is different. Without it small difference will be calculated as whole distribution difference and instead of NaN you will get -0.45623836126629425 in attached example
There was a problem hiding this comment.
Yes, but the workaround injects fake extreme values. Both "before" and "after" seem wrong.
PS. this case is untested, right?
There was a problem hiding this comment.
The only value of this PR is to have a relativeShift correctly calculated, it does not improve classification as I hoped. I will prepare a small PR to fix ShiftedDistributionRegressionTest and decline this one
...in/com/atlassian/performance/tools/report/api/judge/RelativeNonparametricPerformanceJudge.kt
Show resolved
Hide resolved
...ain/kotlin/com/atlassian/performance/tools/report/api/distribution/DistributionComparator.kt
Outdated
Show resolved
Hide resolved
mgrzaslewicz
force-pushed
the
fix-hodges-lehmann
branch
from
92d2c11 to
89b2a3e
Compare
2 participants
I hoped to see classification change for added distribution. As it should be detected as an improvement, while in production we have
no impact:However previous classification was fine for default tolerance. We're just using a different production tolerance in
RegressionCheckITand improvement is below threshold.So at least we have a properly calculated Hodges-Lehmann coming with this change.