-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
MNT Improve error check_array error message when estimator is None #30485
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
MNT Improve error check_array error message when estimator is None #30485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @taharallouche,
thanks a lot for your PR. I like your initiative to make this error message clearer. 😃
I'm not a maintainer, but I am leaving you some comments to work with.
sklearn/utils/validation.py
Outdated
if estimator_name is not None: | ||
raise ValueError( | ||
"Found array with dim %d. %s expected <= 2." | ||
% (array.ndim, estimator_name) | ||
) | ||
raise ValueError("Found array with dim %d. Expected <= 2." % (array.ndim)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already have something in place a few lines up where we define:
estimator_name = _check_estimator_name(estimator)
context = " by %s" % estimator_name if estimator is not None else ""
What about using the context
variable in the error message?
The advantage would be not to require an additional conditional check.
Apart from this (and disregarding how we did this in the past), it would be nice to use f-strings here.
], | ||
) | ||
def test_check_array_allow_nd_errors(X, estimator, expectation) -> None: | ||
with expectation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use the with pytest.raises
context manager directly in the test. For consistence, would you mind doing this here, as well?
), | ||
], | ||
) | ||
def test_check_array_allow_nd_errors(X, estimator, expectation) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to put type hints here.
( | ||
np.array([[1, 2], [3, 4]]), | ||
None, | ||
does_not_raise(), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The does_not_raise
for data with matching shapes is implicitly tested in the other tests, so I would think it is not necessary here.
Thanks for taking the time to review this @StefanieSenger 🙏 I addressed all your suggestions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @taharallouche. I think it looks great.
Two maintainers need to approve this PR.
I have set a quick review label, so it gets people's attention in this busy time.
I believe there is no changelog necessary.
allow_ndarray=False
and estimator is None
allow_ndarray=False
and estimator is NoneCo-authored-by: Loïc Estève <loic.esteve@ymail.com>
Thanks a lot for the improvement! I set-up automerge so this PR will be merged when CI is green. Side-comment: in general you don't need to merge the |
Reference Issues/PRs
I haven't found an issue that is dedicated to this.
What does this implement/fix? Explain your changes.
Hello, this is an attempt to make the error message that
check_array
emits whenX.ndim > 2
in cases whereallow_nd
isFalse
, andestimator
isNone
.Here's the current (on scikit-learn's
main
branch 6cccd99) error message:Here's the output on this branch:
The case where
estimator
is notNone
is unchanged.