-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
FIX improve warning message in _ensure_sparse_format
#27757
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
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 am fine with this changes. We need to change the test and still have an entry in the changelog as a fix since it will change the error message.
sklearn/utils/validation.py
Outdated
@@ -535,7 +535,7 @@ def _ensure_sparse_format( | |||
|
|||
if accept_sparse is False: | |||
raise TypeError( | |||
"A sparse matrix was passed, but dense data is required. Use X.toarray() " | |||
"A sparse matrix was passed, but dense data is required. Use '.toarray()' " |
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.
And I assume that it can be a sparse matrix or array.
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.
Sorry I don't follow?
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 suppose this is about sparse array vs sparse matrix and that you can get an error mentioning "sparse matrix" when you passed a sparse array. More context in #26418.
Maybe use something like "sparse data" or sparse_container.__class__
in the error message?
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.
Ah thank you, I did not know the difference.
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've used 'sparse data' as I think that is enough info but happy to change.
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've added input_name
, if present, to the error message as sometimes more than one argument will be checked (e.g., X and y) and this can help. Happy to change back though.
cc @glemaitre in case this needs re-review.
_ensure_sparse_format
_ensure_sparse_format
Thanks, changes made! @glemaitre |
doc/whats_new/v1.4.rst
Outdated
@@ -534,6 +534,10 @@ Changelog | |||
misdetects the CPU architecture. | |||
:pr:`27614` by :user:`Olivier Grisel <ogrisel>`. | |||
|
|||
- |Fix| Error message in :func:`~utils._ensure_sparse_format` when a sparse matrix was |
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.
Let's document the public function that is impacted.
- |Fix| Error message in :func:`~utils._ensure_sparse_format` when a sparse matrix was | |
- |Fix| Error message in :func:`~utils.check_array` when a sparse matrix was |
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.
LGTM otherwise.
Thanks @lucyleeow |
Changes made, thanks! |
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.
This is still good to me. @lesteve do you want to have a new look at it.
Merging, thanks! |
Thanks for the reviews! |
What does this implement/fix? Explain your changes.
_ensure_sparse_format
is used to checkX
andy
and other parameters but the warning says:This updates the warning to "Use '.'toarray()'" because it is possible that it was not
X
that needs to be converted (and even so, the user has not necessarily named the dataX
).