Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing test flakiness #570

Merged
merged 5 commits into from
Feb 26, 2021
Merged

Fixing test flakiness #570

merged 5 commits into from
Feb 26, 2021

Conversation

sleepy-owl
Copy link
Contributor

The test test_ground_truth_separated_modes sometimes fails non-deterministically. This PR addresses this issue.

To find a solution, I collected samples of value of statistic in the assertion from several test executions and computed the tail distribution. I computed the extreme percentiles to check how high can the values be. The computed percentiles are as follows:

0.9:: 0.09
0.99 :: 0.16
0.999:: 0.20
0.9999 :: 0.30

For this fix, i chose the 99.9th percentile. I think setting the bound using the statistical evaluation might be a good way to ensure the test is not flaky.

Do you guys think this makes sense? Please let me know if this looks good or if you have any other suggestions. Also, here I assume there are no bugs in the code under test.

@yannikschaelte
Copy link
Member

Hi @sleepy-owl , thanks for this contribution! I think checking the test percentiles is the way to go indeed (unless we set the RNG, which we however rather don't want to atm). The Kolmogorov-Smirnov test is unfortunately rather unstable, s.t. the 20% may be the best we can do here I guess.

@yannikschaelte yannikschaelte changed the base branch from master to develop February 24, 2021 08:18
@sleepy-owl
Copy link
Contributor Author

Thanks! Can you please merge this if this looks good?

@sleepy-owl
Copy link
Contributor Author

@yannikschaelte gentle ping! Is there anything else that I should include in the PR? if not, can you please merge the PR?

@yannikschaelte
Copy link
Member

Was waiting for another PR, but will merge soon, thanks!

@sleepy-owl
Copy link
Contributor Author

Thanks!

@yannikschaelte yannikschaelte merged commit 3dc7259 into ICB-DCM:develop Feb 26, 2021
@yannikschaelte yannikschaelte mentioned this pull request Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants