-
Notifications
You must be signed in to change notification settings - Fork 332
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
Update to new sklearn API #397
base: master
Are you sure you want to change the base?
Conversation
Change as per FutureWarning in sklearn, to allow eli5 to be used with sklearn 0.24 and newer.
This update will break compatibility with scikit-learn < 0.20, so it would be necessary to update the requirements and setup.py to require at least this version (eli5 currently allows >=0.18). |
Is the upgrade of the sklearn requirement acceptable, given that 0.18 is now 4 years old and 0.24 has a RC out? |
enforce the upgrade of sklearn
Codecov Report
@@ Coverage Diff @@
## master #397 +/- ##
===========================================
- Coverage 97.32% 80.58% -16.75%
===========================================
Files 49 49
Lines 3142 3142
Branches 585 585
===========================================
- Hits 3058 2532 -526
- Misses 44 569 +525
- Partials 40 41 +1
|
I'd say that yes, it is. Thanks for the PR 👍 |
/eli5/sklearn/text.py in ----> 4 from sklearn.feature_extraction.text import VectorizerMixin # type: ignore should be renamed to _VectorizerMixin |
~/.local/lib/python3.8/site-packages/eli5/sklearn/transform.py from sklearn.feature_selection import SelectorMixin |
@veonua I'm a bit confused what you are suggesting, master reads:
your other comment is also already addressed in this PR |
can you please publish these changes to pip? |
Thanks you! This were merged in eli5-org/eli5#2 and released to PyPI with v0.11 |
Change as per FutureWarning in sklearn, to allow eli5 to be used with sklearn 0.24 and newer. docs
Currently issues FutureWarning