Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

Relax assert for number of non-zero elements in test #191

Merged
merged 2 commits into from
Jan 3, 2022

Conversation

StrikerRUS
Copy link
Contributor

Fix flaky test originally reported in #187 (comment).

I'm not limiting if branch to 32-bit Windows because during preparing this PR I was observing the same test failures on 64-bit Windows as well: https://github.com/scikit-learn-contrib/lightning/runs/4637338353?check_suite_focus=true.

@StrikerRUS
Copy link
Contributor Author

Run 10 consecutive CI jobs for this PR - all were green:

image

https://github.com/scikit-learn-contrib/lightning/actions/runs/1624804596

@StrikerRUS StrikerRUS marked this pull request as ready for review December 26, 2021 21:32
@StrikerRUS StrikerRUS requested a review from mblondel December 26, 2021 21:32
@@ -172,7 +174,10 @@ def test_debiasing_l1l2():
max_iter=20, C=0.01, random_state=0)
clf.fit(mult_csc, mult_target)
assert clf.score(mult_csc, mult_target) > 0.75
assert clf.n_nonzero(percentage=True) == 0.08
if system() == "Windows":
assert 0.07 <= clf.n_nonzero(percentage=True) <= 0.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with clf.n_nonzero(percentage=True) <= 0.1 without the platform-specific test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment! Addressed in 4942a84. I also added lower bound into assertion to check that number is non-negative, if you don't mind.

Copy link
Member

@mblondel mblondel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@StrikerRUS StrikerRUS changed the title Relax assert for number of non-zero elements in test on Windows platform Relax assert for number of non-zero elements in test Jan 3, 2022
@StrikerRUS StrikerRUS merged commit bbdee34 into master Jan 3, 2022
@StrikerRUS StrikerRUS deleted the relax_assert branch January 3, 2022 01:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants