Skip to content
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

abatula
Copy link
Contributor

@abatula 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.

@jnothman
Copy link
Member

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

Thansk.

@abatula
Copy link
Contributor Author

abatula commented Oct 16, 2016

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

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

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():
Copy link
Member

Choose a reason for hiding this comment

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

It's okay to combine these into one test function

Copy link
Member

Choose a reason for hiding this comment

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

And I'd prefer it.


clf = LinearSVC(random_state=0)

grid_search = GridSearchCV(clf, {'C': Cs}, scoring='accuracy')
Copy link
Member

Choose a reason for hiding this comment

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

scoring='accuracy' is unnecessary.

Cs = [.1, 1, 10]

clf = LinearSVC(random_state=0)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

y = np.array([0] * 5 + [1] * 5)

regr = Ridge()

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this is unhelpful whitespace

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

Please add a what's new entry.

@abatula
Copy link
Contributor Author

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?

@amueller
Copy link
Member

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

@@ -19,6 +19,11 @@ New features
Enhancements
............

- Added ``classes_`` attribute to gridSearchCV object that matches the
Copy link
Member

Choose a reason for hiding this comment

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

gridSearchCV should be

:class:`model_selection.GridSearchCV`

@abatula
Copy link
Contributor Author

abatula commented Oct 20, 2016

Fixed reference to gridSearchCV in what's new entry.

@amueller
Copy link
Member

thanks!

@amueller amueller merged commit 707b6f9 into scikit-learn:master Oct 20, 2016
- 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`_.
Copy link
Member

Choose a reason for hiding this comment

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

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

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
…ixes scikit-learn#6298 (scikit-learn#7661)

* Fix issue scikit-learn#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 scikit-learn#7661

* Fixed formatting of update in what's new
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
…ixes scikit-learn#6298 (scikit-learn#7661)

* Fix issue scikit-learn#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 scikit-learn#7661

* Fixed formatting of update in what's new
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
…ixes scikit-learn#6298 (scikit-learn#7661)

* Fix issue scikit-learn#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 scikit-learn#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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GridSearchCV object has no attribute classes_
5 participants