Skip to content

TST Use global_random_seed in sklearn/cluster/tests/test_mean_shift.py #30517

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jan 22, 2025
Merged

TST Use global_random_seed in sklearn/cluster/tests/test_mean_shift.py #30517

merged 6 commits into from
Jan 22, 2025

Conversation

ArturoSbr
Copy link
Contributor

@ArturoSbr ArturoSbr commented Dec 20, 2024 •
edited
Loading

Hi!

I'm opening this PR in regard to this issue.
I only modified test_parallel() by adding an additional argument global_random_seed and using that as the random_state which was previously hardcoded to 11.

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 20, 2024 •
edited
Loading

✔️ Linting Passed

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

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

@ArturoAmorQ ArturoAmorQ changed the title Added global seed arg to test_parallel() TST Use global_random_seed in sklearn/cluster/tests/test_mean_shift.py Dec 20, 2024
@ArturoAmorQ ArturoAmorQ added No Changelog Needed Quick Review For PRs that are quick to review labels Dec 20, 2024
@ArturoAmorQ ArturoAmorQ mentioned this pull request Dec 20, 2024
Open
@ogrisel
Copy link
Member

ogrisel commented Dec 24, 2024 •
edited
Loading

Please push an extra empty commit to this PR to trigger the updated test with all admissible random seeds on all CI runners as documented in #28959.

@ArturoSbr
Copy link
Contributor Author

Please push an extra empty commit to this PR to trigger the updated test with all admissible random seeds on all CI runners as documented in #28959.

Will do!

@ArturoSbr
Copy link
Contributor Author

Hi @ogrisel !
I added the empty commit as requested. All the builds are looking good.
Do I need to do anything else on my end?

@ArturoAmorQ
Copy link
Member

ArturoAmorQ commented Jan 21, 2025 •
edited
Loading

Hi @ArturoSbr, thanks for the PR!
In your case, the commit message should be of the form

empty commit to trigger all CI jobs [all random seeds]
test_parallel

(see for instance #30518).

@lesteve
Copy link
Member

lesteve commented Jan 22, 2025

Merging, thanks a lot!

@lesteve lesteve merged commit fef4701 into scikit-learn:main Jan 22, 2025
29 checks passed
@ArturoAmorQ
Copy link
Member

Congrats on your first merge in scikit-learn @ArturoSbr!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesteve lesteve lesteve approved these changes

@jeremiedbb jeremiedbb Awaiting requested review from jeremiedbb

Assignees
No one assigned
Labels
module:cluster No Changelog Needed Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ArturoSbr @ogrisel @ArturoAmorQ @lesteve