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] Added unit test for adding classes_ property to GridSearchCV, fixes #6298 #7661

Merged
merged 10 commits into from Oct 20, 2016

Conversation

Projects
None yet
5 participants
@abatula
Contributor

abatula commented Oct 13, 2016

Fixes #6298
Completes closed pull request #6449

Added a test to ensure the classes_ property is correclty added to GridSearchCV, and also check that regressors do not have the classes_ attribute. This should complete the previously closed pull request.

unautre and others added some commits Feb 25, 2016

Fix issue #6298
Adds a "classes_" property to BaseSearchCV
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 13, 2016

Member

Please also test that if a regressor is used, hasattr(gscv, 'classes_') is False.

Thansk.

Member

jnothman commented Oct 13, 2016

Please also test that if a regressor is used, hasattr(gscv, 'classes_') is False.

Thansk.

abatula added some commits Oct 16, 2016

@abatula

This comment has been minimized.

Show comment
Hide comment
@abatula

abatula Oct 16, 2016

Contributor

Added a second test to make sure regressors don't have a classes_ attribute.

Contributor

abatula commented Oct 16, 2016

Added a second test to make sure regressors don't have a classes_ attribute.

@jnothman

Otherwise, and with the Travis issues addressed, LGTM.

@@ -785,3 +786,30 @@ def test_parameters_sampler_replacement():
sampler = ParameterSampler(params_distribution, n_iter=7)
samples = list(sampler)
assert_equal(len(samples), 7)
def test_classes__property():

This comment has been minimized.

@jnothman

jnothman Oct 16, 2016

Member

It's okay to combine these into one test function

@jnothman

jnothman Oct 16, 2016

Member

It's okay to combine these into one test function

This comment has been minimized.

@jnothman

jnothman Oct 16, 2016

Member

And I'd prefer it.

@jnothman

jnothman Oct 16, 2016

Member

And I'd prefer it.

Show outdated Hide outdated sklearn/tests/test_grid_search.py
clf = LinearSVC(random_state=0)
grid_search = GridSearchCV(clf, {'C': Cs}, scoring='accuracy')

This comment has been minimized.

@jnothman

jnothman Oct 16, 2016

Member

scoring='accuracy' is unnecessary.

@jnothman

jnothman Oct 16, 2016

Member

scoring='accuracy' is unnecessary.

Show outdated Hide outdated sklearn/tests/test_grid_search.py
Cs = [.1, 1, 10]
clf = LinearSVC(random_state=0)

This comment has been minimized.

@jnothman

jnothman Oct 16, 2016

Member

I don't think this blank line does much good.

@jnothman

jnothman Oct 16, 2016

Member

I don't think this blank line does much good.

This comment has been minimized.

@jnothman

jnothman Oct 16, 2016

Member

In fact, I'd be fine with GridSearchCV(LinearSVC(random_state=0), param_grid)

@jnothman

jnothman Oct 16, 2016

Member

In fact, I'd be fine with GridSearchCV(LinearSVC(random_state=0), param_grid)

Show outdated Hide outdated sklearn/tests/test_grid_search.py
y = np.array([0] * 5 + [1] * 5)
regr = Ridge()

This comment has been minimized.

@jnothman

jnothman Oct 16, 2016

Member

Similarly, this is unhelpful whitespace

@jnothman

jnothman Oct 16, 2016

Member

Similarly, this is unhelpful whitespace

@jnothman jnothman changed the title from [MRG] Added unit test for adding classes_ property to GridSearchCV, fixes #6298 to [MRG+1] Added unit test for adding classes_ property to GridSearchCV, fixes #6298 Oct 16, 2016

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 16, 2016

Member

Please add a what's new entry.

Member

jnothman commented Oct 16, 2016

Please add a what's new entry.

@RPGOne

RPGOne approved these changes Oct 17, 2016

@abatula

This comment has been minimized.

Show comment
Hide comment
@abatula

abatula Oct 18, 2016

Contributor

Merged the two tests into a single test, and fixed the whitespace issues found by Travis CI.

I can't find any information on adding a what's new entry, are there instructions that I'm missing?

Contributor

abatula commented Oct 18, 2016

Merged the two tests into a single test, and fixed the whitespace issues found by Travis CI.

I can't find any information on adding a what's new entry, are there instructions that I'm missing?

@RPGOne

RPGOne approved these changes Oct 18, 2016

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Oct 18, 2016

Member

Just checkout doc/whatsnew.rst. There are no instructions on adding a changelog but it should be pretty self-explanatory.

Member

amueller commented Oct 18, 2016

Just checkout doc/whatsnew.rst. There are no instructions on adding a changelog but it should be pretty self-explanatory.

Show outdated Hide outdated doc/whats_new.rst
@@ -19,6 +19,11 @@ New features
Enhancements
............
- Added ``classes_`` attribute to gridSearchCV object that matches the

This comment has been minimized.

@jnothman

jnothman Oct 19, 2016

Member

gridSearchCV should be

:class:`model_selection.GridSearchCV`
@jnothman

jnothman Oct 19, 2016

Member

gridSearchCV should be

:class:`model_selection.GridSearchCV`
@abatula

This comment has been minimized.

Show comment
Hide comment
@abatula

abatula Oct 20, 2016

Contributor

Fixed reference to gridSearchCV in what's new entry.

Contributor

abatula commented Oct 20, 2016

Fixed reference to gridSearchCV in what's new entry.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Oct 20, 2016

Member

thanks!

Member

amueller commented Oct 20, 2016

thanks!

@amueller amueller merged commit 707b6f9 into scikit-learn:master Oct 20, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
- Added ``classes_`` attribute to :class:`model_selection.GridSearchCV`
that matches the ``classes_`` attribute of ``best_estimator_``. (`#7661
<https://github.com/scikit-learn/scikit-learn/pull/7661>`_) by `Alyssa
Batula`_ and `Dylan Werner-Meier`_.

This comment has been minimized.

@amueller

amueller Oct 20, 2016

Member

You need to add your names to the bottom of the file for the links to work. I'll do that.

@amueller

amueller Oct 20, 2016

Member

You need to add your names to the bottom of the file for the links to work. I'll do that.

afiodorov added a commit to unravelin/scikit-learn that referenced this pull request Apr 25, 2017

[MRG+1] Added unit test for adding classes_ property to GridSearchCV, f…
…ixes #6298 (#7661)

* Fix issue #6298
Adds a "classes_" property to BaseSearchCV

* Added test to ensure classes_ property is added to gridSearch correctly

* Fixed formatting

* Added test to ensure gridSearchCV with a regressor does not have a classes_ attribute

* Fixed whitespace issues

* Combined tests for the new GridSearchSV.classes_ property into a single test.

* Removed trailing whitespace

* Added what's new for pull request #7661

* Fixed formatting of update in what's new

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

[MRG+1] Added unit test for adding classes_ property to GridSearchCV, f…
…ixes #6298 (#7661)

* Fix issue #6298
Adds a "classes_" property to BaseSearchCV

* Added test to ensure classes_ property is added to gridSearch correctly

* Fixed formatting

* Added test to ensure gridSearchCV with a regressor does not have a classes_ attribute

* Fixed whitespace issues

* Combined tests for the new GridSearchSV.classes_ property into a single test.

* Removed trailing whitespace

* Added what's new for pull request #7661

* Fixed formatting of update in what's new

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

[MRG+1] Added unit test for adding classes_ property to GridSearchCV, f…
…ixes #6298 (#7661)

* Fix issue #6298
Adds a "classes_" property to BaseSearchCV

* Added test to ensure classes_ property is added to gridSearch correctly

* Fixed formatting

* Added test to ensure gridSearchCV with a regressor does not have a classes_ attribute

* Fixed whitespace issues

* Combined tests for the new GridSearchSV.classes_ property into a single test.

* Removed trailing whitespace

* Added what's new for pull request #7661

* Fixed formatting of update in what's new

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

[MRG+1] Added unit test for adding classes_ property to GridSearchCV, f…
…ixes #6298 (#7661)

* Fix issue #6298
Adds a "classes_" property to BaseSearchCV

* Added test to ensure classes_ property is added to gridSearch correctly

* Fixed formatting

* Added test to ensure gridSearchCV with a regressor does not have a classes_ attribute

* Fixed whitespace issues

* Combined tests for the new GridSearchSV.classes_ property into a single test.

* Removed trailing whitespace

* Added what's new for pull request #7661

* Fixed formatting of update in what's new
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment