-
Notifications
You must be signed in to change notification settings - Fork 41
MNT fix failing slope test #287
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
MNT fix failing slope test #287
Conversation
skglm/tests/test_penalties.py
Outdated
| clf = SlopeEst( | ||
| alpha=0.01, | ||
| fit_intercept=False, | ||
| scaling = "none", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flake is going to complain about extra spaces around equal here and line below
|
@jolars There are 6 failing not due to Slope, but the last one is because the shape mismatch, you need to squeeze the coefs of libslope: also probably the numerical tolerances are too low |
5f6b580 to
634c597
Compare
Sorry! I manage to miss this due to not having the package installed 😬 I realized that I am also using arguments that are just in the development version. I suppose the reason it's not failing here is that you're installing straight from the github repo and not pypi. Let me push a new version to pypi and then I'll tidy this up. |
|
I believe the SLOPE tests are fixed now. Let me know if this is fine and I'll squash the fixup commits. |
|
@jolars thanks for the PR ! No need to squash on your side, we perform "squash and merge" directly on the UI |
Context of the PR
I think the failing test for SLOPE observed in #282 was due to the Slope estimator
normalizing data by default.
Contributions of the PR
I've turned off normalization in the test now.
(I realize that it might not be completely kosher to normalize in these sklearn
estimators so I'll change the default for the sortedl1 package as well, but
we might as well make it explicit here).
The test is working at least locally for me now.
Checks before merging PR