Skip to content

[MRG+2] DummyClassifier and DummyRegressor issues#8931

Merged
MechCoder merged 1 commit intoscikit-learn:masterfrom
Attractadore:feature-2
Jun 13, 2017
Merged

[MRG+2] DummyClassifier and DummyRegressor issues#8931
MechCoder merged 1 commit intoscikit-learn:masterfrom
Attractadore:feature-2

Conversation

@Attractadore
Copy link
Copy Markdown
Contributor

@Attractadore Attractadore commented May 24, 2017 •
edited by lesteve
Loading

Reference Issue

Fix #8916

What does this implement/fix? Explain your changes.

Removes call to _assert_all_finite in Dummy predict() method and
adds an array check to fit(). Also added tests for this.

Any other comments?

jnothman
jnothman approved these changes Jun 4, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need the change in fit, but LGTM

@jnothman jnothman changed the title [MRG] DummyClassifier and DummyRegressor issues [MRG+1] DummyClassifier and DummyRegressor issues Jun 4, 2017
@amueller
Copy link
Copy Markdown
Member

amueller commented Jun 6, 2017

travis fails

@Attractadore
Copy link
Copy Markdown
Contributor Author

Attractadore commented Jun 7, 2017 •
edited
Loading

What are these tests and what do they check for?

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Jun 8, 2017

You can run the tests with nosetests sklearn from the root folder. Feel free to look at the contributing doc for more details.

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Jun 8, 2017

Please use "Fix #issueNumber" in your PR description, this way the associated issue gets closed automatically when the PR is merged. For more details, look at this.

I have edited your description but please remember to do it next time.

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Jun 8, 2017

If you look at the Travis log, you'll see that the test failing is:

======================================================================
ERROR: sklearn.tests.test_dummy.test_dummy_classifier_on_nan_value
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/testenv/lib/python3.6/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/tests/test_dummy.py", line 608, in test_dummy_classifier_on_nan_value
    y_pred = clf.predict(X)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/dummy.py", line 201, in predict
    proba = self.predict_proba(X)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/dummy.py", line 263, in predict_proba
    X = check_array(X, accept_sparse=['csr', 'csc', 'coo'])
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/utils/validation.py", line 415, in check_array
    _assert_all_finite(array)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/utils/validation.py", line 40, in _assert_all_finite
    " or a value too large for %r." % X.dtype)
ValueError: Input contains NaN, infinity or a value too large for dtype('float64').

It seems like you just added this test so you should understand why it is failing. You can run only this test via:

nosetests sklearn/tests/test_dummy.py -m test_dummy_classifier_on_nan_value

@Attractadore
Copy link
Copy Markdown
Contributor Author

Attractadore commented Jun 8, 2017

Dummy clf predict method called dummy clf predict_proba method, which still checked if X was finite. I've removed the finite check. Should be OK now.

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Jun 8, 2017

Travis failures look genuine

@Attractadore
Copy link
Copy Markdown
Contributor Author

Attractadore commented Jun 8, 2017 •
edited
Loading

Looks like the travis fails on other tests (NuSVC and SVC)

MechCoder
MechCoder approved these changes Jun 13, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@MechCoder MechCoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MechCoder MechCoder changed the title [MRG+1] DummyClassifier and DummyRegressor issues [MRG+2] DummyClassifier and DummyRegressor issues Jun 13, 2017
@MechCoder
Copy link
Copy Markdown
Member

MechCoder commented Jun 13, 2017

It seems like the Appveyor build failures are unrelated. Are we allowed to merge?

@Attractadore
Copy link
Copy Markdown
Contributor Author

Attractadore commented Jun 13, 2017 •
edited
Loading

If you approve, then of cource.

@MechCoder MechCoder merged commit 2c21479 into scikit-learn:master Jun 13, 2017
@MechCoder
Copy link
Copy Markdown
Member

MechCoder commented Jun 13, 2017

Thanks! I merged because of #9111

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
@Attractadore @Sundrique
Removed force_all_finite array checks in DummyClassifier and DummyReg…
18ccb52 
…ressor, added 2 tests (scikit-learn#8931)
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@Attractadore @dmohns
Removed force_all_finite array checks in DummyClassifier and DummyReg…
fa0f5f7 
…ressor, added 2 tests (scikit-learn#8931)
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@Attractadore @dmohns
Removed force_all_finite array checks in DummyClassifier and DummyReg…
79f9e7a 
…ressor, added 2 tests (scikit-learn#8931)
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
@Attractadore @NelleV
Removed force_all_finite array checks in DummyClassifier and DummyReg…
82bf4f2 
…ressor, added 2 tests (scikit-learn#8931)
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@Attractadore @paulha
Removed force_all_finite array checks in DummyClassifier and DummyReg…
eab8740 
…ressor, added 2 tests (scikit-learn#8931)
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
@Attractadore @AishwaryaRK
Removed force_all_finite array checks in DummyClassifier and DummyReg…
269977e 
…ressor, added 2 tests (scikit-learn#8931)
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@Attractadore @maskani-moh
Removed force_all_finite array checks in DummyClassifier and DummyReg…
bb31319 
…ressor, added 2 tests (scikit-learn#8931)
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@Attractadore
Removed force_all_finite array checks in DummyClassifier and DummyReg…
c653a10 
…ressor, added 2 tests (scikit-learn#8931)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman approved these changes

+1 more reviewer

@MechCoder MechCoder MechCoder approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DummyClassifier.predict() does stricter input checking than DummyClassifier.fit()

5 participants

@Attractadore @amueller @lesteve @MechCoder @jnothman