[MRG+1] Remove check_array for X in DummyRegressor/DummyClassifier and replace X.shape[0] with _num_samples #9835

vrishank97 · 2017-09-27T07:07:46Z

Adds dtype="None" and ensure_min_features=0 as params for check_array in DummyRegressor predict.

jnothman

This needs a test.

Perhaps just discard the check_array and use utils.validation._num_samples. Note sure if this will pass the common tests, though.

vrishank97 · 2017-09-27T07:52:59Z

I haven't made tests before. Would adding the code to reproduce the bug from the report to sklearn/tests/test_dummy.py in the form of function suffice?

jnothman · 2017-09-27T08:13:04Z

ideally you would do that, but with a numpy array rather than a pandas DataFrame.

jnothman · 2017-09-27T10:16:14Z

sklearn/tests/test_dummy.py

+
+
+def test_dummy_regressor_on_object_value():
+    X = np.array(['foo', 'bar', 'baz'])


this won't actually have dtype object (check for yourself) so the test is incorrectly named.

Sorry. I'll change it to string.

Changed test name from test_dummy_regressor_on_object_value to test_dummy_regressor_on_string_values.

vrishank97 · 2017-10-06T21:13:46Z

I have changed the test name. @jnothman is there anything else that needs to be changed?

jnothman

LGTM!

qinhanmin2014 · 2017-11-30T03:08:35Z

ping @vrishank97 Some personal concerns:
(1) Why is the check only modified in predict, not in fit? fit and perdict do the same check here.
(2) Seems that the test will pass even in current master. E.g.

X = np.array(['a', 'b', 'c']).reshape(-1, 1)
check_array(X, dtype='numeric') # no error

(3) Seems that the original code snippet in the issue will still get an error.

qinhanmin2014 · 2017-11-30T08:23:43Z

So seems that there's something wrong with check_array (#10229).
@vrishank97 Could you please consider to copy the change to fit (and maybe DummyClassifier) and extend the test accordingly? Thanks.

qinhanmin2014

ping @vrishank97 Could you please take some time to finish it. Otherwise I'll pick it up.
Even with current check_array, I believe we can still construct a regression test using astype("O").

vrishank97 · 2017-12-12T15:53:27Z

Hi. Some work came up earlier. I'll continue my work on this now. Sorry for the delay.

amueller · 2017-12-12T16:24:53Z

We should probably check with a zero feature array as well, right?

vrishank97 · 2017-12-12T17:57:56Z

@qinhanmin2014
I've modified the check_array params for fit and predict in both DummyClassifier and DummyRegressor.
What other tests should I add here?

qinhanmin2014 · 2017-12-13T02:18:27Z

@vrishank97
There's predict_proba in DummyClassifier.
For the test, I think you need to cover all your changes. Also make sure that your test fail on master but success in the PR.

qinhanmin2014 · 2017-12-13T07:14:04Z

sklearn/tests/test_dummy.py

+    y_pred = cls.predict(X)
+    assert_array_equal(y_pred, y_expected)
+
+test_dummy_regressor_on_string_values()


test dummy classifier?

Just made the test. Pushing now

qinhanmin2014 · 2017-12-20T13:37:10Z

@vrishank97 Please also update what's new(docs/whats_new/v0.20.rst) accordingly.

…into patch-3

vrishank97 · 2017-12-20T13:44:29Z

I've updated the what's new section.

qinhanmin2014

@vrishank97 Please avoid making unrelated changes.

qinhanmin2014 · 2017-12-20T13:55:36Z

doc/whats_new/v0.20.rst

@@ -116,6 +116,10 @@ Classifiers and regressors
  :class:`linear_model.BayesianRidge` for weighted linear regression.
  :issue:`10111` by :user:`Peter St. John <pstjohn>`.

+- :class:'dummy.DummyClassifier' and :class:'dummy.DummyRegresssor' now
+  only requires X to be an object with finite length or shape.
+  :issue:'9832' by :user:'Vrishank Bhardwaj <vrishank97>'.


You should use ` instead of ', please follow what others' are doing and check the result from Circle CI.

vrishank97 · 2017-12-20T14:08:55Z

I've removed the typos. I think I also accidentally included a few unrelated commits by others while rebasing. How should I remove them?

qinhanmin2014 · 2017-12-20T14:12:44Z

At least you might recover the code according to the diff.

vrishank97 · 2017-12-20T14:26:09Z

Removed the diffs.

qinhanmin2014

LGTM ping @jnothman

lesteve · 2017-12-21T14:10:40Z

Wow I have no idea about the root reason for this ... but it looks like merging this one reverted some changes in master, probably because of the weird history ...

the diff in this PR changes 3 files
the squashed commit 58ff9b8 changes 7 files, reverting some changes in master ...

I'll fix that in master but this is something to keep in mind (check the history has no quirks) before merging.

lesteve · 2017-12-21T14:16:33Z

Reverted the unwanted changes in master in e824fa1

jnothman · 2017-12-21T14:18:01Z

How strange, thanks for cleaning up after me!

qinhanmin2014 · 2017-12-21T14:23:21Z

Thanks @lesteve Really sorry about that. Also curious about the reason.

lesteve · 2017-12-21T14:45:32Z

Thanks @lesteve Really sorry about that. Also curious about the reason.

No worries! Anyone with a half-baked explanation I am interested ...

jnothman · 2017-12-21T20:40:52Z

Perhaps it's worth notifying support@github.com that there is a bug in squash and merge...?

lesteve · 2017-12-22T07:32:12Z

Maybe, before classifying it as a bug, it would be nice if someone can do a bit of research about

what git commands are executed in the background for the squash and merge
whether similar behaviours have been reported in the past

Add params for check_array in DummyRegressor predict

728b615

jnothman reviewed Sep 27, 2017

View reviewed changes

Fixed typo in check_array params

9c6dd3f

Add test for object val for DummyRegressor

44f559e

vrishank97 changed the title ~~Add params for check_array in DummyRegressor predict~~ [MRG] Add params for check_array in DummyRegressor predict Sep 27, 2017

vrishank97 added 3 commits September 27, 2017 14:52

Remove whitespace and new line

f698ce1

removed whitespace

bfccba7

Removed extra whitespace

9f0a7b7

jnothman reviewed Sep 27, 2017

View reviewed changes

change test name

aadb6f3

Changed test name from test_dummy_regressor_on_object_value to test_dummy_regressor_on_string_values.

jnothman approved these changes Nov 29, 2017

View reviewed changes

jnothman changed the title ~~[MRG] Add params for check_array in DummyRegressor predict~~ [MRG+1] Add params for check_array in DummyRegressor predict Nov 29, 2017

qinhanmin2014 mentioned this pull request Nov 30, 2017

check_array(X, dtype='numeric') should fail if X has strings #10229

Closed

qinhanmin2014 reviewed Dec 12, 2017

View reviewed changes

vrishank97 added 2 commits December 12, 2017 22:07

Modify params for check_array in method fit

4a07455

Add params to check_array in DummyClassifier

de1ab57

vrishank97 added 2 commits December 13, 2017 10:59

Modify check_array params in predict_proba for DummyClassifier

2c89aad

Modified test

f41b4f2

qinhanmin2014 reviewed Dec 13, 2017

View reviewed changes

vrishank97 added 2 commits December 13, 2017 12:44

Modify DummyClassifier test for object values

e493c44

Add test for predict_proba for DummyClassifier

396f4ff

vrishank97 added 7 commits December 20, 2017 18:54

Remove typo

e966540

Remove check_array for X

63f72df

Add test on 3D array for DummyRegressor and DummyClassifier

cb1f2b0

Update parameter description for X

b1b0e2a

Add length = n_samples for X

9140023

Update dummy.py

62ec4e8

Update dummy.py

26c8b21

vrishank97 added 2 commits December 20, 2017 19:09

Update what's new

e163335

Merge branch 'patch-3' of https://github.com/vrishank97/scikit-learn …

0aed384

…into patch-3

qinhanmin2014 reviewed Dec 20, 2017

View reviewed changes

Remove typos

6b3906d

vrishank97 added 4 commits December 20, 2017 19:51

remove diff

7be9f3a

remove diff

2f9bcd2

remove diff

9b8b4a5

remove diff

f24f4fb

qinhanmin2014 approved these changes Dec 20, 2017

View reviewed changes

jnothman approved these changes Dec 20, 2017

View reviewed changes

jnothman merged commit 58ff9b8 into scikit-learn:master Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Remove check_array for X in DummyRegressor/DummyClassifier and replace X.shape[0] with _num_samples #9835

[MRG+1] Remove check_array for X in DummyRegressor/DummyClassifier and replace X.shape[0] with _num_samples #9835

vrishank97 commented Sep 27, 2017

jnothman left a comment

vrishank97 commented Sep 27, 2017

jnothman commented Sep 27, 2017 via email

jnothman Sep 27, 2017

vrishank97 Sep 27, 2017

vrishank97 commented Oct 6, 2017

jnothman left a comment

qinhanmin2014 commented Nov 30, 2017

qinhanmin2014 commented Nov 30, 2017 •

edited

Loading

qinhanmin2014 left a comment

vrishank97 commented Dec 12, 2017

amueller commented Dec 12, 2017

vrishank97 commented Dec 12, 2017

qinhanmin2014 commented Dec 13, 2017

qinhanmin2014 Dec 13, 2017

vrishank97 Dec 13, 2017

qinhanmin2014 commented Dec 20, 2017

vrishank97 commented Dec 20, 2017

qinhanmin2014 left a comment

qinhanmin2014 Dec 20, 2017

vrishank97 Dec 20, 2017

vrishank97 commented Dec 20, 2017

qinhanmin2014 commented Dec 20, 2017

vrishank97 commented Dec 20, 2017

qinhanmin2014 left a comment

lesteve commented Dec 21, 2017 •

edited

Loading

lesteve commented Dec 21, 2017

jnothman commented Dec 21, 2017 via email

qinhanmin2014 commented Dec 21, 2017

lesteve commented Dec 21, 2017

jnothman commented Dec 21, 2017

lesteve commented Dec 22, 2017



		def test_dummy_regressor_on_object_value():
		X = np.array(['foo', 'bar', 'baz'])

[MRG+1] Remove check_array for X in DummyRegressor/DummyClassifier and replace X.shape[0] with _num_samples #9835

[MRG+1] Remove check_array for X in DummyRegressor/DummyClassifier and replace X.shape[0] with _num_samples #9835

Conversation

vrishank97 commented Sep 27, 2017

jnothman left a comment

Choose a reason for hiding this comment

vrishank97 commented Sep 27, 2017

jnothman commented Sep 27, 2017 via email

jnothman Sep 27, 2017

Choose a reason for hiding this comment

vrishank97 Sep 27, 2017

Choose a reason for hiding this comment

vrishank97 commented Oct 6, 2017

jnothman left a comment

Choose a reason for hiding this comment

qinhanmin2014 commented Nov 30, 2017

qinhanmin2014 commented Nov 30, 2017 • edited Loading

qinhanmin2014 left a comment

Choose a reason for hiding this comment

vrishank97 commented Dec 12, 2017

amueller commented Dec 12, 2017

vrishank97 commented Dec 12, 2017

qinhanmin2014 commented Dec 13, 2017

qinhanmin2014 Dec 13, 2017

Choose a reason for hiding this comment

vrishank97 Dec 13, 2017

Choose a reason for hiding this comment

qinhanmin2014 commented Dec 20, 2017

vrishank97 commented Dec 20, 2017

qinhanmin2014 left a comment

Choose a reason for hiding this comment

qinhanmin2014 Dec 20, 2017

Choose a reason for hiding this comment

vrishank97 Dec 20, 2017

Choose a reason for hiding this comment

vrishank97 commented Dec 20, 2017

qinhanmin2014 commented Dec 20, 2017

vrishank97 commented Dec 20, 2017

qinhanmin2014 left a comment

Choose a reason for hiding this comment

lesteve commented Dec 21, 2017 • edited Loading

lesteve commented Dec 21, 2017

jnothman commented Dec 21, 2017 via email

qinhanmin2014 commented Dec 21, 2017

lesteve commented Dec 21, 2017

jnothman commented Dec 21, 2017

lesteve commented Dec 22, 2017

qinhanmin2014 commented Nov 30, 2017 •

edited

Loading

lesteve commented Dec 21, 2017 •

edited

Loading