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

[MRG+2] DummyClassifier and DummyRegressor issues #8931

Merged
merged 1 commit into from Jun 13, 2017

Conversation

Projects
None yet
5 participants
@Attractadore
Copy link
Contributor

Attractadore commented May 24, 2017

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
Copy link
Member

jnothman left a comment

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

This comment has been minimized.

Copy link
Member

amueller commented Jun 6, 2017

travis fails

@Attractadore

This comment has been minimized.

Copy link
Contributor Author

Attractadore commented Jun 7, 2017

What are these tests and what do they check for?

@lesteve

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Member

lesteve commented Jun 8, 2017

Travis failures look genuine

@Attractadore

This comment has been minimized.

Copy link
Contributor Author

Attractadore commented Jun 8, 2017

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

@MechCoder
Copy link
Member

MechCoder left a comment

LGTM!

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

@MechCoder

This comment has been minimized.

Copy link
Member

MechCoder commented Jun 13, 2017

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

@Attractadore

This comment has been minimized.

Copy link
Contributor Author

Attractadore commented Jun 13, 2017

If you approve, then of cource.

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

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MechCoder

This comment has been minimized.

Copy link
Member

MechCoder commented Jun 13, 2017

Thanks! I merged because of #9111

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment