Skip to content

Conversation

dvro
Copy link
Member

@dvro dvro commented Nov 30, 2016

Adapted StratifiedKFold import doing the following:

import sklearn

if hasattr(sklearn, 'model_selection'):
    from sklearn.model_selection import StratifiedKFold
else:
    from sklearn.cross_validation import StratifiedKFold

now, the following command:
from imblearn.ensemble import EasyEnsemble

do not raise the following deprecation warning:

DeprecationWarning: This module was deprecated in version 0.18 in favor of the model_selection module into which all the refactored classes and functions are moved. Also note that the interface of the new CV iterators are different from that of this module. This module will be removed in 0.20.
  "This module will be removed in 0.20.", DeprecationWarning)

@dvro dvro changed the title removing sklearn.cross_validation DeprecationWarning issue #195 [MRG] Resolved #195 - removing sklearn.cross_validation DeprecationWarning issue Nov 30, 2016
@coveralls
Copy link

coveralls commented Nov 30, 2016

Coverage Status

Coverage decreased (-0.03%) to 98.754% when pulling c3900a6 on dvro:deprecation_warning into 7a5afeb on scikit-learn-contrib:master.

@glemaitre
Copy link
Member

@dvro thanks, can you check the problem with appveyor

@coveralls
Copy link

coveralls commented Nov 30, 2016

Coverage Status

Coverage decreased (-0.03%) to 98.754% when pulling 69848ef on dvro:deprecation_warning into 7a5afeb on scikit-learn-contrib:master.

@coveralls
Copy link

coveralls commented Nov 30, 2016

Coverage Status

Coverage decreased (-0.05%) to 98.727% when pulling 2b653d0 on dvro:deprecation_warning into 7a5afeb on scikit-learn-contrib:master.

@dvro
Copy link
Member Author

dvro commented Nov 30, 2016

@chkoar @glemaitre this is just a quickfix, not a definitive solution (IMO, we should support sklearn 0.18.X forward, only).

@glemaitre glemaitre changed the title [MRG] Resolved #195 - removing sklearn.cross_validation DeprecationWarning issue [MRG + 1] Resolved #195 - removing sklearn.cross_validation DeprecationWarning issue Dec 3, 2016
@glemaitre
Copy link
Member

LGTM. @dvro can you had your contribution in the doc/whats_new.rst

@chkoar can you make the merging as MRG+1

@glemaitre glemaitre changed the title [MRG + 1] Resolved #195 - removing sklearn.cross_validation DeprecationWarning issue [MRG+1] Resolved #195 - removing sklearn.cross_validation DeprecationWarning issue Dec 3, 2016
@chkoar
Copy link
Member

chkoar commented Dec 3, 2016

I think that importing the iterator locally would avoid the double check.

@glemaitre
Copy link
Member

Not sure that you follow the pep8 standard doing so.

@chkoar
Copy link
Member

chkoar commented Dec 3, 2016

In general yes. But pep8 it is not a panacea. For these purposes I think that's ok. For example check here a local import https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/base.py#L348

@glemaitre
Copy link
Member

Ok, but we need to keep track somewhere such that we don't forget about it. I would say in the TODO or whatsnew would be the right place.

@chkoar
Copy link
Member

chkoar commented Dec 3, 2016

Another option could be that dvro@4225dff

@coveralls
Copy link

coveralls commented Dec 4, 2016

Coverage Status

Coverage decreased (-0.02%) to 98.787% when pulling 2136348 on dvro:deprecation_warning into 62f6d2f on scikit-learn-contrib:master.

@dvro
Copy link
Member Author

dvro commented Dec 4, 2016

@chkoar that's a cool approach, however, if one tries to the cv_split method anywhere else and the the check_X_y doesn't pass, it won't be clear to the user that this is the problem. maybe using the if else inside the cv_split method?

@chkoar
Copy link
Member

chkoar commented Dec 4, 2016

I don't mind. Make your changes to see the diff. However, if there is a need in the future to use this approach elsewhere we'll pull out this function to a utils or something module. It is a private function.

@coveralls
Copy link

coveralls commented Dec 4, 2016

Coverage Status

Coverage decreased (-0.05%) to 98.761% when pulling c8260ed on dvro:deprecation_warning into 62f6d2f on scikit-learn-contrib:master.

@dvro dvro changed the title [MRG+1] Resolved #195 - removing sklearn.cross_validation DeprecationWarning issue [WIP] Resolved #195 - removing sklearn.cross_validation DeprecationWarning issue Dec 4, 2016
@glemaitre
Copy link
Member

LGTM.

@glemaitre
Copy link
Member

@dvro @chkoar If you can in the whatsnew then we can merge this PR

@chkoar
Copy link
Member

chkoar commented Dec 8, 2016

@glemaitre he has changed the title of the PR back to [WIP] so lets wait

@glemaitre glemaitre merged commit c8260ed into scikit-learn-contrib:master Dec 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants