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] Add classes_ parameter to hyperparameter CV classes #8295

Merged
merged 5 commits into from Feb 10, 2017

Conversation

Projects
None yet
3 participants
@stephen-hoover
Contributor

stephen-hoover commented Feb 4, 2017

Reference Issue

Closes #8290 .

What does this implement/fix? Explain your changes.

In BaseSearchCV (superclass of GridSearchCV and RandomizedSearchCV), add a classes_ parameter which surfaces the classes_ parameter of the best_estimator_. Other parts of the scikit-learn code (e.g. sklearn.model_selection.cross_val_predict) as well as users expect this property to be present on fitted classifiers.

Any other comments?

When making this PR, I realized that there had already been a nearly identical PR, #7661 . That PR added the classes_ property in the deprecated sklearn.grid_search module. This PR adds it to the sklearn.model_selection module. The only difference is that I've added a

self._check_is_fitted("classes_")

call, with the intent of raising an error which is clearer than an AttributeError referencing best_estimator_.

The What's New file claims that this change has already happened, so there doesn't seem to be anything to change there.

ENH Add classes_ parameter to hyperparameter CV classes
In ``BaseSearchCV`` (superclass of ``GridSearchCV`` and ``RandomizedSearchCV``), add a ``clases_`` parameter which surfaces the ``classes_`` parameter of the ``best_estimator_``. Other parts of the scikit-learn code (e.g. ``cross_val_predict``) as well as users expect this property to be present on fitted classifiers.
@stephen-hoover

This comment has been minimized.

Show comment
Hide comment
@stephen-hoover

stephen-hoover Feb 5, 2017

Contributor

I don't know why Travis claims to have failed. It looks like all of the tests succeeded.

Contributor

stephen-hoover commented Feb 5, 2017

I don't know why Travis claims to have failed. It looks like all of the tests succeeded.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Feb 6, 2017

Member

Travis is broken atm: travis-ci/travis-ci#7264

Member

jnothman commented Feb 6, 2017

Travis is broken atm: travis-ci/travis-ci#7264

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Feb 6, 2017

Member

Whoops that we failed to port across #7661 to model_selection!! Could you please port across the test too?

Member

jnothman commented Feb 6, 2017

Whoops that we failed to port across #7661 to model_selection!! Could you please port across the test too?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Feb 6, 2017

Member

Thanks!

Member

jnothman commented Feb 6, 2017

Thanks!

Show outdated Hide outdated sklearn/model_selection/tests/test_validation.py
@@ -914,7 +915,7 @@ def test_cross_val_predict_sparse_prediction():
assert_array_almost_equal(preds_sparse, preds)
def test_cross_val_predict_with_method():
def run_cross_val_predict_with_method(est):

This comment has been minimized.

@jnothman

jnothman Feb 6, 2017

Member

we often call helpers in tests check_*

@jnothman

jnothman Feb 6, 2017

Member

we often call helpers in tests check_*

Show outdated Hide outdated sklearn/model_selection/tests/test_search.py
grid_search = GridSearchCV(clf, {'foo_param': [1, 2, 3]})
grid_search.fit(X, y)
assert_array_equal(grid_search.classes_, np.unique(y))

This comment has been minimized.

@jnothman

jnothman Feb 6, 2017

Member

Please add a test to check that hasattr(gscv, 'classes_') is false given a regressor?

@jnothman

jnothman Feb 6, 2017

Member

Please add a test to check that hasattr(gscv, 'classes_') is false given a regressor?

@stephen-hoover

This comment has been minimized.

Show comment
Hide comment
@stephen-hoover

stephen-hoover Feb 6, 2017

Contributor

Thanks, @jnothman ! I copied the test from #7661 and added a couple of other checks on the classes_ attribute. I also renamed the test helper function in test_validation.py to check_cross_val_predict_with_method.

Contributor

stephen-hoover commented Feb 6, 2017

Thanks, @jnothman ! I copied the test from #7661 and added a couple of other checks on the classes_ attribute. I also renamed the test helper function in test_validation.py to check_cross_val_predict_with_method.

@jnothman

LGTM, thanks. And thanks for catching our omission.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Feb 9, 2017

Codecov Report

Merging #8295 into master will increase coverage by <.01%.

@@            Coverage Diff             @@
##           master    #8295      +/-   ##
==========================================
+ Coverage   94.74%   94.74%   +<.01%     
==========================================
  Files         342      342              
  Lines       60711    60735      +24     
==========================================
+ Hits        57519    57543      +24     
  Misses       3192     3192
Impacted Files Coverage Δ
sklearn/model_selection/_search.py 97.88% <100%> (+0.02%)
sklearn/model_selection/tests/test_validation.py 98.23% <100%> (+0.01%)
sklearn/model_selection/tests/test_search.py 99.3% <100%> (+0.01%)

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 3a0ea19...6b136a2. Read the comment docs.

codecov bot commented Feb 9, 2017

Codecov Report

Merging #8295 into master will increase coverage by <.01%.

@@            Coverage Diff             @@
##           master    #8295      +/-   ##
==========================================
+ Coverage   94.74%   94.74%   +<.01%     
==========================================
  Files         342      342              
  Lines       60711    60735      +24     
==========================================
+ Hits        57519    57543      +24     
  Misses       3192     3192
Impacted Files Coverage Δ
sklearn/model_selection/_search.py 97.88% <100%> (+0.02%)
sklearn/model_selection/tests/test_validation.py 98.23% <100%> (+0.01%)
sklearn/model_selection/tests/test_search.py 99.3% <100%> (+0.01%)

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 3a0ea19...6b136a2. Read the comment docs.

@stephen-hoover

This comment has been minimized.

Show comment
Hide comment
@stephen-hoover

stephen-hoover Feb 9, 2017

Contributor

@jnothman , I merged master, since GitHub reported a conflict in _search.py. It wasn't really a conflict; you can see that the diff in that file is still the same. AppVeyor ran this time. What's the next step?

Contributor

stephen-hoover commented Feb 9, 2017

@jnothman , I merged master, since GitHub reported a conflict in _search.py. It wasn't really a conflict; you can see that the diff in that file is still the same. AppVeyor ran this time. What's the next step?

@jnothman jnothman changed the title from [MRG] Add classes_ parameter to hyperparameter CV classes to [MRG+1] Add classes_ parameter to hyperparameter CV classes Feb 9, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Feb 9, 2017

Member

Usually I'd wait for another reviewer, just to be sure I didn't miss anything.

Could you add a note in what's new to say this was a bug fix because we failed to implement the change properly for 0.18?

Member

jnothman commented Feb 9, 2017

Usually I'd wait for another reviewer, just to be sure I didn't miss anything.

Could you add a note in what's new to say this was a bug fix because we failed to implement the change properly for 0.18?

@stephen-hoover

This comment has been minimized.

Show comment
Hide comment
@stephen-hoover

stephen-hoover Feb 9, 2017

Contributor

Okay. The change in #7661 was for v0.19; it hasn't been in a release yet. The text in the existing What's New entry describes what this PR does, so I added the PR number rather than making a new entry.

Contributor

stephen-hoover commented Feb 9, 2017

Okay. The change in #7661 was for v0.19; it hasn't been in a release yet. The text in the existing What's New entry describes what this PR does, so I added the PR number rather than making a new entry.

Show outdated Hide outdated doc/whats_new.rst
@@ -68,7 +68,8 @@ Enhancements
- Added ``classes_`` attribute to :class:`model_selection.GridSearchCV`
that matches the ``classes_`` attribute of ``best_estimator_``. :issue:`7661`
by :user:`Alyssa Batula <abatula>` and :user:`Dylan Werner-Meier <unautre>`.
and :issue:`8295` by :user:`Alyssa Batula <abatula>`,

This comment has been minimized.

@lesteve

lesteve Feb 9, 2017

Member

Maybe add the class grid_search.GridSearchCV, grid_search.RandomizedSearchCV and model_selection.RandomizedSearchCV to make this entry more exact.

@lesteve

lesteve Feb 9, 2017

Member

Maybe add the class grid_search.GridSearchCV, grid_search.RandomizedSearchCV and model_selection.RandomizedSearchCV to make this entry more exact.

This comment has been minimized.

@stephen-hoover

stephen-hoover Feb 9, 2017

Contributor

Done.

@stephen-hoover

stephen-hoover Feb 9, 2017

Contributor

Done.

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Feb 10, 2017

Member

LGTM, thanks a lot, merging!

Member

lesteve commented Feb 10, 2017

LGTM, thanks a lot, merging!

@lesteve lesteve merged commit eee8be4 into scikit-learn:master Feb 10, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 94.74%)
Details
codecov/project 94.74% (+<.01%) compared to 3a0ea19
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stephen-hoover

This comment has been minimized.

Show comment
Hide comment
@stephen-hoover

stephen-hoover Feb 10, 2017

Contributor

Thank you!

Contributor

stephen-hoover commented Feb 10, 2017

Thank you!

sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

@Przemo10 Przemo10 referenced this pull request Mar 17, 2017

Closed

update fork (#1) #8606

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 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

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

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