FIX Raise proper error for sparse dataframe with mixed dtypes #17992

alexshacked · 2020-07-25T22:50:21Z

Fixes #17945.
Currently, function check_array() in validation.py throws a confusing exception while validating:

A sparse DataFrame with mixed numeric types
A sparse DataFrame with object types.
The error happens because for this cases the DataFrame creates a coo_matrix with dtype('object'), and when
trying to create a csr_matrix from this coo_matrix scipy will raise a ValueError.
This scipy Exception is not very informative for the scikit-learn user that gets this exception while trying to fit an
estimator with a sparse DataFrame.

The fix is to handle this two cases in check_array by looking at the coo_matrix and issue a clear Exception to the caller.
The fix includes a uninittest that handles all the intput types scenarios that can generate this issue.
The unnittest will fail if run in the version prior to this fix.

…ixed types (scikit-learn#17945)

…7945)

…it-learn#17945)

thomasjpfan

Thank you for the PR @alexshacked

I was wondering if we can allow for the sparse conversion to happen, and then check the dtype. If it is a object dtype, raise an error regarding pandas extensions arrays etc.

To get more attention on a PR, consider updating the title. (It will be used as the commit message when this PR gets merged).

alexshacked · 2020-07-27T21:52:57Z

Hey @thomasjpfan. In this fix we analyze the coo_matrix , we got from
array.sparse.to_coo() - line 570 in validation.py
and if it is of dtype('object') we raise the new Exception.
If we did not do that, the next line is :
array = _ensure_sparse_format(...
and this would throw us the original Exception while calling coo_matrix.to_csr()
So, in short we can let the code create the coo_matrix and analyze it's type (and this is what we are actually doing),
but we cannot go to the next step and create the csr_matrix from the coo_matrix because this would throw the original scipy exception

thomasjpfan

I see that the test is trying to cover many test cases, but I think we only need to test the example demonstrated in the issue.

thomasjpfan · 2020-07-27T23:16:50Z

sklearn/utils/validation.py

+            raise ValueError(
+                "Pandas DataFrame with sparse extention arrays generated "
+                "a sparse coo_matrix with dtype 'np.object'\n which "
+                "scipy cannot convert to a CSR or a CSC. {}".format(mxg))


For now, I do not think we need to be specific about the cause. I think a message such as:

Pandas DataFrame with mixed sparse extension arrays generated a sparse matrix with object dtype which can not be converted to a scipy sparse matrix

hey @thomasjpfan. There are two scenarios, that can result in scipy coo_matrix.to_csr throwing the Exception:
TypeError: no supported conversion for types: (dtype('O'),)
a. check_array receives a DataFrame with columns of different numeric types
b. check_array receives a data frame where all the columns are the same type but this type is str or bytes or object

tf = pd.DataFrame({'col1': pd.arrays.SparseArray([0, 1, 0], dtype='str'), 'col2': pd.arrays.SparseArray([1, 0, 1], dtype='str')}) a = check_array(tf, **{'accept_sparse': ['csr', 'csc'], 'ensure_min_features': 2}) ... raise TypeError('no supported conversion for types: %r' % (args,)) TypeError: no supported conversion for types: (dtype('O'),)

In this fix I try to handle both cases since they both throw the same scipy exception.

For scenario b, I don't think we can display the message:
Pandas DataFrame with mixed sparse extension arrays generated a sparse matrix with object dtype which can not be converted to a scipy sparse matrix.

So you propose that we won't handle scenario b, meaning that we let the system throw the original exception?
TypeError: no supported conversion for types: (dtype('O'),)

So you propose that we won't handle scenario b, meaning that we let the system throw the original exception?

Yes.

I do not think check_array should expect SparseArrays with strings or objects. Raising an error for the common use case of mixed numerical data as specified in #17945 would be enough.

I am trying to avoid too much pandas specific code in the test suite. I think a simple test for mixed numerical sparse arrays, would be sufficient to resolve the original issue.

Got it. Will do. Thanks a lot.

…cikit-learn#17945)

alexshacked · 2020-08-03T18:50:44Z

I changed the fix to support only the mixed numeric types scenario.
When running circleci I noticed that some tests use the latest pandas version 1.1.0, while other tests still work with pandas 1.0.5.
Pandas 1.1.0 fixes the mixed numerics type issue. So from this version and beyond we do not need this fix.
My current delivery tests pandas version and expects an Exception to be raised only if pandas version is 1.0.5 and below.
My question is, do we still want to add this fix and increase the implementation of check _array() considering that an year from now version 1.0.5 and below will probably not be very frequent? Pandas are releasing a new version nearly every month.

alexshacked · 2020-08-05T16:28:20Z

Hey @thomasjpfan. I chanded the fix to handle only the mixed types scenario. Meanwhile pandas released version 1.1.0 which fixes this issue. Please, see my full comment above.

thomasjpfan · 2020-08-05T18:31:40Z

sklearn/utils/tests/test_validation.py

@@ -1213,3 +1213,85 @@ def test_check_sparse_pandas_sp_format(sp_format):
    assert sp.issparse(result)
    assert result.format == sp_format
    assert_allclose_dense_sparse(sp_mat, result)
+
+
+def make_types_table():


I would prefer not to have this function at all, we do not need to check every single pandas dtype combinations.

thomasjpfan · 2020-08-05T18:32:23Z

sklearn/utils/tests/test_validation.py

+@pytest.mark.parametrize('dt_name',
+                         ['bool', 'float', 'int', 'uint'])


We can explicitly list out the combinations we are about there

@pytest.mark.parametrize('ntype1, ntype2', [ ("int32", "long"), ... ]) def test_check_pandas_sparse_invalid(ntype1, ntype2): pd = pytest.importskip("pandas") df = pd.DataFrame(...) with pytest.raises(ValueError, ...): check_array(df, ...) @pytest.mark.parametrize('ntype1, ntype2', [ ("int", "long"), ... ]) def test_check_pandas_sparse_valid(ntype1, ntype2): # make sure valid combinations result in the expected sparse matrix.

I changed the tests accordingly

thomasjpfan · 2020-08-05T18:36:03Z

sklearn/utils/validation.py

+            if is_sparse_df_with_mixed_types(array_orig):
+                raise ValueError(
+                    "Pandas DataFrame with mixed sparse extension arrays "
+                    "generated a sparse matrix\n with object dtype which "


Suggested change

"generated a sparse matrix\n with object dtype which "

"generated a sparse matrix with object dtype which "

…it-learn#17945)

glemaitre

A couple of style changes and a bit more check required in the tests.

sklearn/utils/validation.py

sklearn/utils/tests/test_validation.py

glemaitre · 2020-08-21T12:57:49Z

sklearn/utils/tests/test_validation.py

+    check_array(df, **{'accept_sparse': ['csr', 'csc'],
+                       'ensure_min_features': 2})


Suggested change

check_array(df, **{'accept_sparse': ['csr', 'csc'],

'ensure_min_features': 2})

check_array(df, **{'accept_sparse': ['csr', 'csc']})

We should also check the output by checking that we get a numpy array with the expected dtype?

Or at least that we have a single dtype

check_array returns a scipy csr_matrix. Scipy sparse matrices like numpy arrays always contain elements of the same type. The type is found in the matrix member dtype. So, one type is guaranteed.
I can test that the dtype of the csr_matrix is as expected. Will do.

glemaitre · 2020-08-21T12:58:05Z

sklearn/utils/validation.py

+                raise ValueError(
+                    "Pandas DataFrame with mixed sparse extension arrays "
+                    "generated a sparse matrix with object dtype which "
+                    "can not be converted to a scipy sparse matrix")


Suggested change

"can not be converted to a scipy sparse matrix")

"can not be converted to a scipy sparse matrix."

)

Shall we provide some hints to solve the issue? One might my want to convert manually to a single dtype if possible?

sklearn/utils/tests/test_validation.py

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

…17358)

glemaitre

LGTM. @thomasjpfan do you want to have a look at this and potentially merge it.

doc/whats_new/v0.24.rst

sklearn/utils/tests/test_validation.py

glemaitre · 2020-08-25T09:05:54Z

FYI I check codecov. I am sure that we covered any change that we did in this PR.

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

alexshacked · 2020-08-25T10:19:15Z

Thanks a lot @glemaitre

thomasjpfan

Minor comments. Otherwise LGTM

sklearn/utils/tests/test_validation.py

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

alexshacked · 2020-08-25T16:59:37Z

@thomasjpfan Thank you very much for guiding me through this issue.

…-learn#17992) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

alexshacked added 5 commits July 23, 2020 16:20

issue clear error message in check_array for sparse DataFrames with m…

1e54095

…ixed types (scikit-learn#17945)

Added unittest for numeric types (scikit-learn#17945)

f6996b6

Added unittest for object types (scikit-learn#17945)

d552c70

Refactored common code (scikit-learn#17945)

8d8fbe1

Common code serves both numbers test and objects test (scikit-learn#1…

9972985

…7945)

github-actions bot added the module:utils label Jul 25, 2020

alexshacked added 8 commits July 26, 2020 01:59

fixed lint errors

7266fcc

fix comment

947581b

removed void types

db5b17f

Handling numpy 1.19.1 DeprecationWarning on conversion to dtype (scik…

939d0de

…it-learn#17945)

Remove accidental lines (scikit-learn#17945)

ac41d79

Refactored DataFrame filtering

a6f06b6

lint fixes fix_17945

fd51f55

codecove fix_17945

45ec4af

thomasjpfan reviewed Jul 27, 2020

View reviewed changes

alexshacked changed the title ~~Fix 17945~~ Raise ValueError with clear error message in check_array for sparse DataFrames with mixed types. Fixes issue #17945 Jul 27, 2020

thomasjpfan reviewed Jul 27, 2020

View reviewed changes

alexshacked added 8 commits August 3, 2020 16:02

Handle only the case of pandas mixed numerical sparse arrays

5c078d0

lint (scikit-learn#17945)

4719d47

pandas 1.1.0 fixed the mixed numeric types issue

3db551d

lint (scikit-learn#17945)

c0d0a1c

support pandas mixed numeric type error for pandas older than 1.1.0 (s…

fd0ff25

…cikit-learn#17945)

Exception message (scikit-learn#17945)

95954e5

codecov (scikit-learn#17945)

9b8b4dd

lint (scikit-learn#17945)

765f5e7

thomasjpfan reviewed Aug 5, 2020

View reviewed changes

deleted redundant file

bf45eca

alexshacked added 2 commits August 19, 2020 17:16

merged with upstream (scikit-learn#17945)

2798ec4

test that invalid behaviour is fixed starting from pandas 1.1.0 (scik…

ef2c4ee

…it-learn#17945)

glemaitre reviewed Aug 21, 2020

View reviewed changes

glemaitre changed the title ~~Raise ValueError with clear error message in check_array for sparse DataFrames with mixed types. Fixes issue #17945~~ FIX raise proper error for sparse dataframe with mixed data type Aug 21, 2020

alexshacked and others added 10 commits August 24, 2020 14:38

Update sklearn/utils/validation.py

abc3257

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update sklearn/utils/tests/test_validation.py

a137399

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update sklearn/utils/tests/test_validation.py

111a9ae

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update sklearn/utils/tests/test_validation.py

ba0c0ea

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update sklearn/utils/tests/test_validation.py

b1fb4e5

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update sklearn/utils/tests/test_validation.py

da086dd

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

ensure that the result csr_matrix is of expected dtype (scikit-learn#…

5cdfc3a

…17358)

Added a hit to the mixed types exception (scikit-learn#17358)

8700a29

added comments to test_check_pandas_sparse_invalid (scikit-learn#17358)

c84d93b

Merge remote-tracking branch 'upstream/master' into fix_17945

34cda20

glemaitre approved these changes Aug 25, 2020

View reviewed changes

doc/whats_new/v0.24.rst Outdated Show resolved Hide resolved

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

glemaitre added this to TO BE MERGED in Guillaume's pet Aug 25, 2020

Update doc/whats_new/v0.24.rst

fcea511

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

cmarmo added the Waiting for Reviewer label Aug 25, 2020

thomasjpfan approved these changes Aug 25, 2020

View reviewed changes

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

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

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

alexshacked and others added 3 commits August 25, 2020 19:57

Update sklearn/utils/tests/test_validation.py

c107227

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Update sklearn/utils/tests/test_validation.py

1fbc6a1

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Update sklearn/utils/tests/test_validation.py

f60e8e2

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan changed the title ~~FIX raise proper error for sparse dataframe with mixed data type~~ FIX Raise proper error for sparse dataframe with mixed dtypes Aug 25, 2020

thomasjpfan merged commit 0111511 into scikit-learn:master Aug 25, 2020
5 checks passed

glemaitre moved this from TO BE MERGED to MERGED in Guillaume's pet Aug 26, 2020

alfaro96 mentioned this pull request Aug 27, 2020

BUG The test_check_pandas_sparse_valid uses platform-dependent definitions #18271

Closed

FIX Raise proper error for sparse dataframe with mixed dtypes #17992

FIX Raise proper error for sparse dataframe with mixed dtypes #17992

alexshacked commented Jul 25, 2020 •

edited

thomasjpfan left a comment

alexshacked commented Jul 27, 2020

thomasjpfan left a comment

thomasjpfan Jul 27, 2020

alexshacked Aug 1, 2020 •

edited

thomasjpfan Aug 1, 2020

alexshacked Aug 1, 2020

alexshacked commented Aug 3, 2020 •

edited

alexshacked commented Aug 5, 2020

thomasjpfan Aug 5, 2020

alexshacked Aug 7, 2020

thomasjpfan Aug 5, 2020

alexshacked Aug 8, 2020

thomasjpfan Aug 5, 2020

alexshacked Aug 7, 2020

glemaitre left a comment

glemaitre Aug 21, 2020

glemaitre Aug 21, 2020

glemaitre Aug 21, 2020

alexshacked Aug 24, 2020

alexshacked Aug 24, 2020

glemaitre Aug 21, 2020

glemaitre Aug 21, 2020

alexshacked Aug 24, 2020

glemaitre left a comment

glemaitre commented Aug 25, 2020

alexshacked commented Aug 25, 2020

thomasjpfan left a comment

alexshacked commented Aug 25, 2020

		@pytest.mark.parametrize('dt_name',
		['bool', 'float', 'int', 'uint'])

	"generated a sparse matrix\n with object dtype which "
	"generated a sparse matrix with object dtype which "

		check_array(df, **{'accept_sparse': ['csr', 'csc'],
		'ensure_min_features': 2})

	"can not be converted to a scipy sparse matrix")
	"can not be converted to a scipy sparse matrix."
	)

FIX Raise proper error for sparse dataframe with mixed dtypes #17992

FIX Raise proper error for sparse dataframe with mixed dtypes #17992

Conversation

alexshacked commented Jul 25, 2020 • edited

thomasjpfan left a comment

Choose a reason for hiding this comment

alexshacked commented Jul 27, 2020

thomasjpfan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshacked Aug 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshacked commented Aug 3, 2020 • edited

alexshacked commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

glemaitre commented Aug 25, 2020

alexshacked commented Aug 25, 2020

thomasjpfan left a comment

Choose a reason for hiding this comment

alexshacked commented Aug 25, 2020

alexshacked commented Jul 25, 2020 •

edited

alexshacked Aug 1, 2020 •

edited

alexshacked commented Aug 3, 2020 •

edited