Skip to content

Conversation

lorentzenchr
Copy link
Member

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?

Copy link

github-actions bot commented Aug 9, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: d91a345. Link to the linter CI: here

@lorentzenchr lorentzenchr added the Quick Review For PRs that are quick to review label Aug 9, 2025
Copy link
Member

@virchan virchan left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @lorentzenchr!

Note to reviewers:

  1. _scale_alpha_inplace is not used anywhere.
  2. test_path_parameters is covered by check_estimators_overwrite_params
  3. test_read_only_buffer is covered by check_inplace_ensure_writeable

@virchan virchan added the Waiting for Second Reviewer First reviewer is done, need a second one! label Aug 12, 2025
@lorentzenchr
Copy link
Member Author

@virchan Thanks for point out the reasons for the redundant tests.

@lorentzenchr lorentzenchr enabled auto-merge (squash) August 12, 2025 20:10
@lorentzenchr lorentzenchr disabled auto-merge August 12, 2025 20:10
@lorentzenchr
Copy link
Member Author

@betatim Could you merge? @virchan is a core contributor and his +1 counts, but github needs someone with merge rights.

else:
# To avoid silent errors in case of refactoring
raise NotImplementedError
@pytest.mark.filterwarnings("ignore::sklearn.exceptions.ConvergenceWarning")
Copy link
Member

Choose a reason for hiding this comment

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

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 :-/

Copy link
Member

Choose a reason for hiding this comment

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

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

@lorentzenchr
Copy link
Member Author

@betatim I fixed the merge conflict.

@lorentzenchr lorentzenchr merged commit 42cbd9d into scikit-learn:main Aug 14, 2025
36 checks passed
@lorentzenchr lorentzenchr deleted the cd_test_cleanup branch August 14, 2025 08:52
lucyleeow pushed a commit to lucyleeow/scikit-learn that referenced this pull request Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:linear_model No Changelog Needed Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants