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+1] Check estimator pairwise #9701

Merged
merged 71 commits into from Nov 13, 2017

Conversation

Projects
None yet
3 participants
@GKjohns
Contributor

GKjohns commented Sep 7, 2017

Reference Issue

Fixes issue #9580.

What does this implement/fix? Explain your changes.

This allows check_estimator() to work on estimators that have the _pairwise attribute set to True. test_check_estimator_pairwise() does this by calling it on a SVC with a precomputed kernel.

The I created a function that checks for the attribute in the estimator being tested and creates a precomputed Gram matrix if the estimator accepts pairwise input. In all of the applicable estimator checks, I wrap the data X in this function

Any other comments?

Some of the checks either a) can't accept precomputed kernels or b) are set to fail in cases that don't apply to precomputed kernels. In those cases I skipped the test.

@jnothman

I've not checked how complete this is, but I like the direction it's heading in!

Show outdated Hide outdated sklearn/base.py Outdated
Show outdated Hide outdated sklearn/base.py Outdated
Show outdated Hide outdated sklearn/utils/estimator_checks.py Outdated
Show outdated Hide outdated sklearn/utils/estimator_checks.py Outdated
Show outdated Hide outdated sklearn/base.py Outdated
Show outdated Hide outdated sklearn/utils/estimator_checks.py Outdated
@@ -251,3 +252,9 @@ def __init__(self):
check_no_fit_attributes_set_in_init,
'estimator_name',
NonConformantEstimator)

This comment has been minimized.

@jnothman

jnothman Sep 7, 2017

Member

PEP8: extra blank line required

@jnothman

jnothman Sep 7, 2017

Member

PEP8: extra blank line required

Show outdated Hide outdated sklearn/utils/estimator_checks.py Outdated
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 18, 2017

Codecov Report

Merging #9701 into master will decrease coverage by <.01%.
The diff coverage is 94.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9701      +/-   ##
==========================================
- Coverage   96.19%   96.19%   -0.01%     
==========================================
  Files         336      336              
  Lines       62739    62781      +42     
==========================================
+ Hits        60353    60392      +39     
- Misses       2386     2389       +3
Impacted Files Coverage Δ
sklearn/utils/tests/test_estimator_checks.py 96.83% <100%> (+0.14%) ⬆️
sklearn/neighbors/regression.py 100% <100%> (ø) ⬆️
sklearn/neighbors/tests/test_neighbors.py 99.43% <66.66%> (-0.15%) ⬇️
sklearn/utils/estimator_checks.py 93.21% <94.82%> (-0.1%) ⬇️
sklearn/ensemble/gradient_boosting.py 95.76% <0%> (-0.45%) ⬇️
sklearn/decomposition/pca.py 95.04% <0%> (-0.15%) ⬇️
sklearn/ensemble/tests/test_gradient_boosting.py 96.27% <0%> (-0.04%) ⬇️
sklearn/linear_model/stochastic_gradient.py 98.17% <0%> (ø) ⬆️
sklearn/feature_selection/base.py 94.79% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abb43c1...3116c23. Read the comment docs.

codecov bot commented Sep 18, 2017

Codecov Report

Merging #9701 into master will decrease coverage by <.01%.
The diff coverage is 94.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9701      +/-   ##
==========================================
- Coverage   96.19%   96.19%   -0.01%     
==========================================
  Files         336      336              
  Lines       62739    62781      +42     
==========================================
+ Hits        60353    60392      +39     
- Misses       2386     2389       +3
Impacted Files Coverage Δ
sklearn/utils/tests/test_estimator_checks.py 96.83% <100%> (+0.14%) ⬆️
sklearn/neighbors/regression.py 100% <100%> (ø) ⬆️
sklearn/neighbors/tests/test_neighbors.py 99.43% <66.66%> (-0.15%) ⬇️
sklearn/utils/estimator_checks.py 93.21% <94.82%> (-0.1%) ⬇️
sklearn/ensemble/gradient_boosting.py 95.76% <0%> (-0.45%) ⬇️
sklearn/decomposition/pca.py 95.04% <0%> (-0.15%) ⬇️
sklearn/ensemble/tests/test_gradient_boosting.py 96.27% <0%> (-0.04%) ⬇️
sklearn/linear_model/stochastic_gradient.py 98.17% <0%> (ø) ⬆️
sklearn/feature_selection/base.py 94.79% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abb43c1...3116c23. Read the comment docs.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 18, 2017

Member

Only flake8 is failing now ...

Member

jnothman commented Sep 18, 2017

Only flake8 is failing now ...

Show outdated Hide outdated sklearn/utils/estimator_checks.py Outdated
Show outdated Hide outdated sklearn/utils/estimator_checks.py Outdated
Show outdated Hide outdated sklearn/utils/tests/test_estimator_checks.py Outdated
@GKjohns

This comment has been minimized.

Show comment
Hide comment
@GKjohns

GKjohns Oct 30, 2017

Contributor

@amueller fixed, knn works for sparse X where metric != 'precomputed now

Contributor

GKjohns commented Oct 30, 2017

@amueller fixed, knn works for sparse X where metric != 'precomputed now

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Oct 30, 2017

Member

thanks, lgtm. @jnothman, still good?

Member

amueller commented Oct 30, 2017

thanks, lgtm. @jnothman, still good?

@jnothman

Otherwise looks good.

Could you please add a new heading in the changelog called "Changes to estimator checks" and note this change there. I'll add a blurb there eventually.

Show outdated Hide outdated sklearn/utils/estimator_checks.py Outdated

GKjohns added some commits Nov 1, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Nov 1, 2017

Member

I'm happy to merge once you've updated the changelog in doc/whats_new/v0.20.rst

Member

jnothman commented Nov 1, 2017

I'm happy to merge once you've updated the changelog in doc/whats_new/v0.20.rst

@jnothman

Sorry my last look has uncovered a few minor things

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
Show outdated Hide outdated sklearn/neighbors/regression.py Outdated
Show outdated Hide outdated sklearn/neighbors/tests/test_neighbors.py Outdated
Show outdated Hide outdated sklearn/utils/estimator_checks.py Outdated

GKjohns added some commits Nov 6, 2017

remove assert_raises() for precomputed metric in test_kneighbors_regr…
…essor_sparse(). Already checked using test_check_estimator_pairwise()
@jnothman

It may still be a good idea to assert that predicting on a sparse precomputed matrix in knn raises a ValueError, but the test where you put that assertion didn't run it with metric=precomputed.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Nov 9, 2017

Member

Happy to merge when green

Member

jnothman commented Nov 9, 2017

Happy to merge when green

@GKjohns

This comment has been minimized.

Show comment
Hide comment
@GKjohns

GKjohns Nov 13, 2017

Contributor

@jnothman is there anything else I should do before you merge?

Contributor

GKjohns commented Nov 13, 2017

@jnothman is there anything else I should do before you merge?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Nov 13, 2017

Member

I'd sort of expected someone in a different timezone would hit the green button!

Thanks for the ping, and for your work!

Member

jnothman commented Nov 13, 2017

I'd sort of expected someone in a different timezone would hit the green button!

Thanks for the ping, and for your work!

@jnothman jnothman merged commit de0581a into scikit-learn:master Nov 13, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 97.22% of diff hit (target 96.19%)
Details
codecov/project 96.2% (+<.01%) compared to effd75d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details

@GKjohns GKjohns deleted the GKjohns:check_estimator_pairwise branch Nov 13, 2017

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Nov 15, 2017

Member

Congrats @GKjohns :)

Member

amueller commented Nov 15, 2017

Congrats @GKjohns :)

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