[MRG+1] Bug in BaseSearchCV.inverse_transform #8348
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8348 +/- ##
==========================================
+ Coverage 94.75% 94.75% +<.01%
==========================================
Files 342 342
Lines 60801 60809 +8
==========================================
+ Hits 57609 57617 +8
Misses 3192 3192
Continue to review full report at Codecov.
|
|
Please give me some pointers, how I shall start writing test. |
|
You should write a test in sklearn/model_selection/tests/test_search.py. Then you should add an entry on the bug fix list doc/whats_new.rst |
|
In test_search.py there is a dummy MockClassifier() class that is used for testing. Should adding a dummy |
|
Sure you could.
…On 17 February 2017 at 12:30, akshay0724 ***@***.***> wrote:
In test_search.py
<https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/model_selection/tests/test_search.py>
there is a dummy MockClassifier() class that is used for testing. Should
adding a dummy inverse_transform() and transform() function to it and
using them in a test looks correct ?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8348 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz65P07SNu9675kXPTtEt4GIZuu5Rgks5rdPgZgaJpZM4L-_LH>
.
|
8000
sklearn/model_selection/tests/test_search.py
Outdated
|
|
||
| grid_search.fit(X, y) | ||
| inverse_transform = grid_search.inverse_transform(X) | ||
| assert_array_equal(X, inverse_transform) |
There was a problem hiding this comment.
Rather than assuming what inverse_transform is I think it would be better to check that inverse_transform(transform(X)) == X.
You could implement transform to be X + self.foo_param and inverse_transform to be X - self.foo_param for example.
There was a problem hiding this comment.
I have committed these changes please have a look. Thanks
doc/whats_new.rst
Outdated
| - Fixed a bug where :class:`sklearn.linear_model.RandomizedLasso` and | ||
| :class:`sklearn.linear_model.RandomizedLogisticRegression` breaks for | ||
| sparse input. | ||
| :issue:`8259` by :user:`Aman Dalmia <dalmia>`. | ||
|
|
||
|
|
There was a problem hiding this comment.
Best practice: don't change code unless there is a very good reason to. Remove the new line you added.
There was a problem hiding this comment.
Sorry, that was by mistake.
There was a problem hiding this comment.
No problem, but it's always good to review your diff before and/or after commit to reduce diff noise like this.
|
I pushed a small naming improvement in your branch, merging thanks a lot ! |
|
Thanks for merging @lesteve. |
Reference Issue
Fixes #8344
What does this implement/fix? Explain your changes.
Code for inverse transform function in BaseSearchCV was written incorrect, I have changed it from
.transform(Xt)to.inverse_transform(Xt)Any other comments?