-
-
Notifications
You must be signed in to change notification settings - Fork 26k
[MRG+1] Fix inverse_transform in deprecated GridSearchCV #8860
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] Fix inverse_transform in deprecated GridSearchCV #8860
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advise you to look closely at https://github.com/scikit-learn/scikit-learn/pull/8348/files and do something very similar. This will be a lot quicker to review this way.
In particular you can update the existing whats_new.rst entry saying that the same bug was fixed for sklearn.grid_search.GridSearchCV.
sklearn/tests/test_grid_search.py
Outdated
@@ -71,9 +71,16 @@ def fit(self, X, Y): | |||
def predict(self, T): | |||
return T.shape[0] | |||
|
|||
def transform(self,T): | |||
T = T - 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Python, you don't need ;
at the end of the lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, will look at it!
sklearn/tests/test_grid_search.py
Outdated
T = T - 2; | ||
return T; | ||
|
||
def inverse_transform(self,T): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8 -> def inverse_transform(self, T):
sklearn/tests/test_grid_search.py
Outdated
# Test the transform operations | ||
transformed = grid_search.transform(X) | ||
reverse_transformed = grid_search.inverse_transform(transformed) | ||
assert_equal(np.array_equal(reverse_transformed, X), True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to use assert_array_equal()
instead of assert_equal(np.array_equal())
Tips: when you want to compare with True
, there is assert_true
sklearn.utils.testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, did not know those functions existed, will look more closely into your linked url.
sklearn/tests/test_grid_search.py
Outdated
@@ -71,9 +71,16 @@ def fit(self, X, Y): | |||
def predict(self, T): | |||
return T.shape[0] | |||
|
|||
def transform(self,T): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8 ->def transform(self, T):
+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except the change in whats_new
that needs to be fixed, LGTM
doc/whats_new.rst
Outdated
@@ -172,6 +172,10 @@ Enhancements | |||
:issue:`7674` by:user:`Yichuan Liu <yl565>`. | |||
|
|||
Bug fixes | |||
|
|||
- Fixed a bug where :func:`sklearn.grid_search.BaseSearchCV.inverse_transform` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not add it between Bug fixes
and .........
.
By the way, you could probably merge this entry with the one of Akshay0724
4 entries below, which was exactly the same on a different file.
sklearn/tests/test_grid_search.py
Outdated
@@ -802,4 +815,4 @@ def test_classes__property(): | |||
# Test that regressors do not have a classes_ attribute | |||
grid_search = GridSearchCV(Ridge(), {'alpha': [1.0, 2.0]}) | |||
grid_search.fit(X, y) | |||
assert_false(hasattr(grid_search, 'classes_')) | |||
assert_false(hasattr(grid_search, 'classes_')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not remove the last empty line
LGTM merging, thanks a lot! |
Reference Issue
Fixes #8846
What does this implement/fix? Explain your changes.
Calling the right function in the method inverse_transform(self,Xt) in grid_search.py
Any other comments?
First try contributing!