Skip to content

ENH/DOC clearer sample weight validation error msg #31873

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

Conversation

kapslock123
Copy link
Contributor

Reference Issues/PRs

Fixes #31712

What does this implement/fix? Explain your changes.

sklearn.utils._check_sample_weight used to raise
ValueError: Sample weights must be 1D array or scalar,
which was misleading (scalars are accepted) and gave no clue about the bad input.

This PR replaces that with a string
"Sample weights must be a scalar or a 1-D array-like of length n_samples. "
f"Got ndim={sample_weight.ndim} instead."

Any other comments?

Copy link

github-actions bot commented Aug 3, 2025

✔️ Linting Passed

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

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

Copy link
Member

@adrinjalali adrinjalali 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 @kapslock123

@adrinjalali adrinjalali changed the title Bugfix/31712 clearer sample weight msg ENH/DOC clearer sample weight validation error msg Aug 4, 2025
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
@kapslock123
Copy link
Contributor Author

I can't work out why one of the required test is failing, is there anything I can change to fix it?

test name:
scikit-learn.scikit-learn
scikit-learn.scikit-learnFailing after 55m — Build #20250804.32 failed

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

The filename still doesn't match the PR name, although my comment explicitly had the new filename in it.

@kapslock123
Copy link
Contributor Author

Oops I've edited it now I thought the convention was to have issue number followed by bug/ enhancement etc .rst

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@jeremiedbb jeremiedbb 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 @kapslock123

@kapslock123
Copy link
Contributor Author

LGTM. Thanks @kapslock123

Amazing thank you for approving my PR. Is there anything else I need to do?

@jeremiedbb jeremiedbb enabled auto-merge (squash) August 8, 2025 10:17
@jeremiedbb jeremiedbb merged commit 6fd23fc into scikit-learn:main Aug 8, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect error message in _check_sample_weight
3 participants