MNT Improve error check_array error message when estimator is None #30485

taharallouche · Dec 14, 2024

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 when X.ndim > 2 in cases where allow_nd is False, and estimator is None.

Here's the current (on scikit-learn's main branch 6cccd99) error message:

X = np.array([[[1,2],[1,2]],[[1,2],[1,2]]])
check_array(X, allow_nd=False)

# ValueError: Found array with dim 3. None expected <= 2.

Here's the output on this branch:

X = np.array([[[1,2],[1,2]],[[1,2],[1,2]]])
check_array(X, allow_nd=False)

# ValueError: Found array with dim 3. Expected <= 2.

The case where estimator is not None is unchanged.

github-actions · Dec 14, 2024

✔️ Linting Passed

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

_{Generated for commit: 5d1dc4e. Link to the linter CI: here}

StefanieSenger

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.

StefanieSenger · Dec 16, 2024

sklearn/utils/validation.py

+            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))


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.

StefanieSenger · Dec 16, 2024

sklearn/utils/tests/test_validation.py

+    ],
+)
+def test_check_array_allow_nd_errors(X, estimator, expectation) -> None:
+    with expectation:


We usually use the with pytest.raises context manager directly in the test. For consistence, would you mind doing this here, as well?

StefanieSenger · Dec 16, 2024

sklearn/utils/tests/test_validation.py

+        ),
+    ],
+)
+def test_check_array_allow_nd_errors(X, estimator, expectation) -> None:


No need to put type hints here.

StefanieSenger · Dec 16, 2024

sklearn/utils/tests/test_validation.py

+        (
+            np.array([[1, 2], [3, 4]]),
+            None,
+            does_not_raise(),
+        ),


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.

taharallouche · Dec 17, 2024

Thanks for taking the time to review this @StefanieSenger 🙏 I addressed all your suggestions

StefanieSenger

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.

sklearn/utils/tests/test_validation.py

Co-authored-by: Loïc Estève <loic.esteve@ymail.com>

lesteve · Dec 20, 2024

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 main branch into your PR branch. One case when you need to do it is when there are conflicts.

fix: check array error message if estimator is none

116021b

github-actions bot added the module:utils label Dec 14, 2024

StefanieSenger reviewed Dec 16, 2024

View reviewed changes

fix: use context variable and rework test

dd5de4a

StefanieSenger added the Quick Review For PRs that are quick to review label Dec 18, 2024

StefanieSenger approved these changes Dec 18, 2024

View reviewed changes

lesteve changed the title ~~fix: check array error message if estimator is none~~ MNT Improve error check_array error_message when allow_ndarray=False and estimator is None Dec 20, 2024

lesteve added the No Changelog Needed label Dec 20, 2024

lesteve changed the title ~~MNT Improve error check_array error_message when allow_ndarray=False and estimator is None~~ MNT Improve error check_array error_message when estimator is None Dec 20, 2024

lesteve changed the title ~~MNT Improve error check_array error_message when estimator is None~~ MNT Improve error check_array error message when estimator is None Dec 20, 2024

lesteve reviewed Dec 20, 2024

View reviewed changes

sklearn/utils/tests/test_validation.py Outdated Show resolved Hide resolved

taharallouche and others added 2 commits December 20, 2024 10:19

review: test the error message with default allow_nd

d51e7be

Co-authored-by: Loïc Estève <loic.esteve@ymail.com>

Merge branch 'main' into check-array-ndim-error-msg

5d1dc4e

lesteve approved these changes Dec 20, 2024

View reviewed changes

lesteve merged commit 72b35a4 into scikit-learn:main Dec 20, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

MNT Improve error check_array error message when estimator is None #30485

MNT Improve error check_array error message when estimator is None #30485

Uh oh!

taharallouche commented Dec 14, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Dec 14, 2024 •

edited

Loading

Uh oh!

StefanieSenger left a comment

Uh oh!

StefanieSenger Dec 16, 2024 •

edited

Loading

Uh oh!

StefanieSenger Dec 16, 2024 •

edited

Loading

Uh oh!

StefanieSenger Dec 16, 2024

Uh oh!

StefanieSenger Dec 16, 2024

Uh oh!

taharallouche commented Dec 17, 2024

Uh oh!

StefanieSenger left a comment •

edited

Loading

Uh oh!

Uh oh!

lesteve commented Dec 20, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Search code, repositories, users, issues, pull requests...

Uh oh!

MNT Improve error check_array error message when estimator is None #30485

MNT Improve error check_array error message when estimator is None #30485

Uh oh!

Conversation

taharallouche commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Uh oh!

github-actions bot commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

StefanieSenger left a comment

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

taharallouche commented Dec 17, 2024

Uh oh!

StefanieSenger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lesteve commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taharallouche commented Dec 14, 2024 •

edited

Loading

github-actions bot commented Dec 14, 2024 •

edited

Loading

StefanieSenger Dec 16, 2024 •

edited

Loading

StefanieSenger Dec 16, 2024 •

edited

Loading

StefanieSenger left a comment •

edited

Loading

lesteve commented Dec 20, 2024 •

edited

Loading