TST/MNT clean up some tests in coordinate descent #31909
Conversation
There was a problem hiding this comment.
LGTM! Thanks, @lorentzenchr!
Note to reviewers:
_scale_alpha_inplaceis not used anywhere.test_path_parametersis covered bycheck_estimators_overwrite_paramstest_read_only_bufferis covered bycheck_inplace_ensure_writeable
|
@virchan Thanks for point out the reasons for the redundant tests. |
| else: | ||
| # To avoid silent errors in case of refactoring | ||
| raise NotImplementedError | ||
| @pytest.mark.filterwarnings("ignore::sklearn.exceptions.ConvergenceWarning") |
There was a problem hiding this comment.
There was a conflict which I resolved, but now this new test has appeared in the diff. I'm confused by that because it looked like this was a change from main but if that is true, why does it show up in the diff?
@lorentzenchr does it make sense to you or can you resolve the conflict (locally)? Sorry for the mess :-/
There was a problem hiding this comment.
I think we should just delete this test again. I can't find it in https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/linear_model/tests/test_coordinate_descent.py so we don't need it. Still puzzled why it showed up when resolving the conflict
46472b4 to
d91a345
Compare
|
@betatim I fixed the merge conflict. |
Reference Issues/PRs
None
What does this implement/fix? Explain your changes.
Remove some duplicated tests, mostly covered by the common estimator tests in
estimators_checks.py.Any other comments?