Skip to content

TST use global_random_seed in sklearn/metrics/tests/test_classification.py #30851

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 18 commits into from
Mar 3, 2025
Merged

TST use global_random_seed in sklearn/metrics/tests/test_classification.py #30851

merged 18 commits into from
Mar 3, 2025

Conversation

Rishab260
Copy link
Contributor

Reference Issues/PRs

towards #22827

What does this implement/fix? Explain your changes.

I added the global_random_seed fixture to the tests:-

  • test_matthews_corrcoef_against_numpy_corrcoef
  • test_matthews_corrcoef_against_jurman
  • test_matthews_corrcoef
  • test_matthews_corrcoef_multiclass
  • test_matthews_corrcoef_overflow
  • test_classification_metric_pos_label_types

Any other comments?

This is my first PR ever, feedback and guidance is highly appreciated. Looking forward to contributing to this project more and more in the coming days. Thanks in advance!

Rishab260 and others added 10 commits February 18, 2025 21:17
@Rishab260
add global_random_seed to test_matthews_corrcoef_against_numpy_corrcoef
dba519d
@Rishab260
add global_random_seed to test_matthews_corrcoef_against_jurman
f6137f7
@Rishab260
add global_random_seed to test_matthews_corrcoef
81740a3
@Rishab260
add global_random_seed to test_matthews_corrcoef_multiclass
d8b2584
@Rishab260
add global_random_seed to test_matthews_corrcoef_overflow
2e14c8f
@Rishab260
add global_random_seed to test_classification_metric_pos_label_types
459de87
@Rishab260
add global_random_seed to make_prediction
af92257
@Rishab260
Undo add global_random_seed to make_prediction
734ab6c
@Rishab260
test_classification [all random seeds]
282b3af 
test_matthews_corrcoef_against_numpy_corrcoef
test_matthews_corrcoef_against_jurman
test_matthews_corrcoef
test_matthews_corrcoef_multiclass
test_classification_metric_pos_label_types
@Rishab260
Merge branch 'scikit-learn:main' into test_classification
f0596c4
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 18, 2025 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 371d82d. Link to the linter CI: here

yuanx749
yuanx749 suggested changes Feb 22, 2025
View reviewed changes
Rishab260 and others added 2 commits February 22, 2025 17:07
@Rishab260 @yuanx749
Update sklearn/metrics/tests/test_classification.py
05f25e8 
Co-authored-by: Xiao Yuan <yuanx749@gmail.com>
@Rishab260 @yuanx749
Update sklearn/metrics/tests/test_classification.py
f342409 
Co-authored-by: Xiao Yuan <yuanx749@gmail.com>
@Rishab260
Copy link
Contributor Author

Thanks @yuanx749 for suggesting the changes, changes have been applied, please let me know if this looks good to you now?

@Rishab260 Rishab260 requested a review from yuanx749 February 22, 2025 14:13
yuanx749
yuanx749 reviewed Feb 22, 2025
View reviewed changes
Copy link
Contributor

@yuanx749 yuanx749 left a comment

Choose a reason for hiding this comment

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

LGTM. Let's wait for maintainers to review and approve.

OmarManzoor
OmarManzoor reviewed Mar 2, 2025
View reviewed changes
Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Rishab260
Can you please push a commit with [all random seeds] and the tests you updated similar to how it is mentioned in #28959

@Rishab260
Copy link
Contributor Author

Rishab260 commented Mar 2, 2025 •
edited
Loading

Thanks for the PR @Rishab260 Can you please push a commit with [all random seeds] and the tests you updated similar to how it is mentioned in #28959

Hi @OmarManzoor , 282b3af is this not it? Do I need to do this once more?

@OmarManzoor
Copy link
Contributor

Hi @OmarManzoor , 282b3af is this not it? Do I need to do this once more?

I think you may have to push this once more as I can't really see the CI results with that commit.

@Rishab260
Copy link
Contributor Author

Hi @OmarManzoor , 282b3af is this not it? Do I need to do this once more?

I think you may have to push this once more as I can't really see the CI results with that commit.

Okay, doing this again. thanks.

Rishab260 and others added 2 commits March 2, 2025 13:24
@Rishab260
test_classification [all random seeds]
025b9c7 
test_matthews_corrcoef_against_numpy_corrcoef
test_matthews_corrcoef_against_jurman
test_matthews_corrcoef
test_matthews_corrcoef_multiclass
test_matthews_corrcoef_overflow
@Rishab260
Merge branch 'main' into test_classification
2f154b3
@OmarManzoor
Copy link
Contributor

@Rishab260 The global seeds commit should be the latest one. Since you merged main after that commit the previous CI run gets cancelled.

@Rishab260
Copy link
Contributor Author

@Rishab260 The global seeds commit should be the latest one. Since you merged main after that commit the previous CI run gets cancelled.

Acknowledged, doing this.

@Rishab260
test_classification [all random seeds]
371d82d 
test_matthews_corrcoef_against_numpy_corrcoef
test_matthews_corrcoef_against_jurman
test_matthews_corrcoef
test_matthews_corrcoef_multiclass
test_matthews_corrcoef_overflow
@Rishab260 Rishab260 requested a review from OmarManzoor March 2, 2025 08:51
OmarManzoor
OmarManzoor approved these changes Mar 3, 2025
View reviewed changes
Copy link
Contributor

@OmarManzoor OmarManzoor 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 @Rishab260

@OmarManzoor OmarManzoor merged commit 0a39bb5 into scikit-learn:main Mar 3, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuanx749 yuanx749 yuanx749 requested changes

@OmarManzoor OmarManzoor OmarManzoor approved these changes

Assignees
No one assigned
Labels
module:metrics No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Rishab260 @OmarManzoor @yuanx749 @StefanieSenger