[MRG] BUG: add support for non numeric values in MissingIndicator by glemaitre · Pull Request #13046 · scikit-learn/scikit-learn

glemaitre · 2019-01-26T21:29:52Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Add support to non-numeric values in MissingIndicator. I flag this PR as BUG since SimpleImputer is already supporting non-numeric data and that we cannot use the MissingIndicator with the SimpleImputer in 0.20. I think that it should go in 0.20.3

I added 3 tests:

check that we are raising an error when the numpy string type is raising an error;
check that we can use string using object dtype;
check that the missing indicator is working in conjunction with the SimpleImputer.

…e object

jeremiedbb · 2019-01-28T09:40:50Z

sklearn/utils/estimator_checks.py

+        msg = "argument must be a string or a number"
+        assert_raises_regex(TypeError, msg, estimator.fit, X, y)
+    else:
+        # If the estimator support strings passed as object dtype,


Actually X[0, 0] = {'foo': 'bar'} is not a string entry, but a dict entry. So I think the test was correct. (Otherwise it would have been needed to modify it for the support of strings in the SimpleImputer).

The error is also not raised for SimpleImputer with the strategy supporting mean. The difference is that the common tests are using default parameter which is the mean strategy for the SimpleImputer.

In [9]: imputer = SimpleImputer(strategy='most_frequent') In [11]: X = np.array([[{'foo': 'bar'}, 'b', 'c']], dtype=object) In [12]: imputer.fit(X) Out[12]: SimpleImputer(copy=True, fill_value=None, missing_values=nan, strategy='most_frequent', verbose=0)

I have to check what is going wrong in check array which is the one that should raise this error.

The error is raised by np.asarray only when we make the conversion. So it means that we neeed to raise such error when dtype=object.

I think check_array needs an update to properly deal with non-numeric input

I think check_array needs an update to properly deal with non-numeric input

See also #12148

jnothman

Should we be testing the error produced when string dtype is passed? Otherwise looks good.

jnothman · 2019-01-27T12:35:30Z

sklearn/impute.py

+                        force_all_finite=force_all_finite)
+        _check_inputs_dtype(X, self.missing_values)
+        if X.dtype.kind not in ("i", "u", "f", "O"):
+            raise ValueError("Missing indicator does not support data with "


Missing indicator -> MissingIndicator

jnothman · 2019-01-29T01:46:44Z

sklearn/utils/estimator_checks.py

 ALLOW_NAN = ['Imputer', 'SimpleImputer', 'MissingIndicator',
             'MaxAbsScaler', 'MinMaxScaler', 'RobustScaler', 'StandardScaler',
             'PowerTransformer', 'QuantileTransformer']
+SUPPORT_STRING = ['SimpleImputer', 'MissingIndicator']


surely OneHotEncoder, OrdinalEncoder and meta-estimators will belong here too.

Can we do that in another PR. I think that we should also factorize the input validation as well. It is quite redundant and we could have a common test then.

Yes, it probably belongs in estimator tags... (perhaps post-#8022)

glemaitre · 2019-01-29T09:46:52Z

Should we be testing the error produced when string dtype is passed? Otherwise looks good.

It is covered there:
https://github.com/scikit-learn/scikit-learn/pull/13046/files#diff-065d21984eda92d7583f963f704a9876R514

jnothman

Fine. If you use "MissingIndicator" instead of "missing indicator" this is good to merge by me.

jnothman · 2019-01-29T10:11:52Z

sklearn/tests/test_impute.py

+      "'sparse' has to be a boolean or 'auto'"),
+     (np.array([['a', 'b'], ['c', 'a']], dtype=str),
+      np.array([['a', 'b'], ['c', 'a']], dtype=str),
+      {}, "Missing indicator does not support data with dtype")]


This will break

jorisvandenbossche

Minor comments, looks good to me

doc/whats_new/v0.20.rst

jorisvandenbossche · 2019-01-30T14:59:15Z

sklearn/tests/test_impute.py

              err_msg=err_msg.format(True))
    assert_ae(X_trans, X_true, err_msg=err_msg.format(True))

-


pep8, does not need to be removed

sklearn/tests/test_impute.py

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

jorisvandenbossche · 2019-01-30T21:24:21Z

Just need to merge master for the pep8 failure

…kit-learn#13010)

See scikit-learn#13074

…n#13054)

…t-learn#11295)

…3029)

…ts (scikit-learn#13064)

…scikit-learn#12989)

glemaitre · 2019-01-30T21:42:33Z

and I mess up my merging (basically I rebase again) but this should be fine with the final squashing.

qinhanmin2014

We need to refactor some input validations and some common tests, but I agree that we can do these things in another PR.

…arn#13046)

…cikit-learn#13046)" This reverts commit 29c51c4.

…arn#13046)

EHN: add support for non numeric values in MissingIndicator

1176d66

glemaitre changed the title ~~EHN: add support for non numeric values in MissingIndicator~~ BUG: add support for non numeric values in MissingIndicator Jan 26, 2019

DOC: add whats new entry

00d7cb5

glemaitre changed the title ~~BUG: add support for non numeric values in MissingIndicator~~ [MRG] BUG: add support for non numeric values in MissingIndicator Jan 26, 2019

FIX/TST: estimator supporting string should not raise error with dtyp…

21eff01

…e object

jeremiedbb reviewed Jan 28, 2019

View reviewed changes

DOC: fix the comment on the reason of not raising an error

42d5b36

jnothman reviewed Jan 29, 2019

View reviewed changes

jnothman approved these changes Jan 29, 2019

View reviewed changes

FIX: change error message

507ed3a

jnothman reviewed Jan 29, 2019

View reviewed changes

glemaitre mentioned this pull request Jan 29, 2019

RFC/MAINT: refactor and common tests for estimator supporting string (object dtype) #13063

Open

TST: fix the error message match

eb4765c

jorisvandenbossche reviewed Jan 30, 2019

View reviewed changes

jorisvandenbossche and others added 2 commits January 30, 2019 17:19

Update doc/whats_new/v0.20.rst

d4e7778

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

TST: additional test with mixed type string/nan/None

7fb7599

jorisvandenbossche approved these changes Jan 30, 2019

View reviewed changes

thomasjpfan and others added 11 commits January 30, 2019 22:38

[MRG] Configure lgtm.yml for CPP (scikit-learn#13044)

ec0ab84

FIX float16 overflow on accumulator operations in StandardScaler (sci…

3721a61

…kit-learn#13010)

TST Use random state to initialize MLPClassifier. (scikit-learn#12892)

f38c8d1

API Deprecate externals.six (scikit-learn#12916)

4078baa

DOC Remove outdated doc in KBinsDiscretizer (scikit-learn#13047)

a0a99d8

DOC Remove outdated doc in KBinsDiscretizer

e92eb1f

See scikit-learn#13074

EXA Improve example plot_svm_anova.py (scikit-learn#11731)

dd73bab

DOC Correct TF-IDF formula in TfidfTransformer comments. (scikit-lear…

de9630f

…n#13054)

FIX an issue w/ large sparse matrix indices in CountVectorizer (sciki…

1d8c534

…t-learn#11295)

DOC More details about the attributes in MinMaxScaler (scikit-learn#1…

f483247

…3029)

DOC Clean up the advanced installation doc to remove python < 3.5 par…

42ed142

…ts (scikit-learn#13064)

zjpoh and others added 5 commits January 30, 2019 22:38

API NMF and non_negative_factorization have inconsistent default init (…

9445eb1

…scikit-learn#12989)

MAINT: pin flake8 to stable version (scikit-learn#13066)

a95b08e

EXA: fix xlabel and ylabel in plot_cv_digits.py (scikit-learn#13067)

193ed4e

MAINT: remove flake8 pinning in circle ci (scikit-learn#13071)

85fe5f6

Merge remote-tracking branch 'origin/master' into is/13035

0b5a96d

qinhanmin2014 approved these changes Feb 3, 2019

View reviewed changes

qinhanmin2014 added this to the 0.20.3 milestone Feb 3, 2019

qinhanmin2014 merged commit 1ae1f1d into scikit-learn:master Feb 3, 2019

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 6, 2019

FIX add support for non numeric values in MissingIndicator (scikit-le…

c5b57db

…arn#13046)

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 7, 2019

FIX add support for non numeric values in MissingIndicator (scikit-le…

ee3246d

…arn#13046)

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Feb 19, 2019

FIX add support for non numeric values in MissingIndicator (scikit-le…

2594d91

…arn#13046)

jnothman mentioned this pull request Feb 19, 2019

Release 0.20.3 #13186

Merged

17 tasks

jeremiedbb mentioned this pull request Mar 22, 2019

[MRG] Parameter for stacking missing indicator into imputer #12583

Merged

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

FIX add support for non numeric values in MissingIndicator (scikit-le…

29c51c4

…arn#13046)

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "FIX add support for non numeric values in MissingIndicator (s…

457f128

…cikit-learn#13046)" This reverts commit 29c51c4.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "FIX add support for non numeric values in MissingIndicator (s…

f9ac7e7

…cikit-learn#13046)" This reverts commit 29c51c4.

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

FIX add support for non numeric values in MissingIndicator (scikit-le…

2505eb8

…arn#13046)

		err_msg=err_msg.format(True))
		assert_ae(X_trans, X_true, err_msg=err_msg.format(True))

Uh oh!

Conversation

glemaitre commented Jan 26, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Jan 29, 2019

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jorisvandenbossche commented Jan 30, 2019

Uh oh!

glemaitre commented Jan 30, 2019

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

glemaitre commented Jan 26, 2019 •

edited

Loading