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

Update to new sklearn API #397

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

icfly2
Copy link

@icfly2 icfly2 commented Dec 9, 2020

Change as per FutureWarning in sklearn, to allow eli5 to be used with sklearn 0.24 and newer. docs

Currently issues FutureWarning

  File "/app/model/model.py", line 3, in <module>
    import eli5
  File "/usr/local/lib/python3.7/site-packages/eli5/__init__.py", line 13, in <module>
    from .sklearn import explain_weights_sklearn, explain_prediction_sklearn
  File "/usr/local/lib/python3.7/site-packages/eli5/sklearn/__init__.py", line 3, in <module>
    from .explain_weights import (
  File "/usr/local/lib/python3.7/site-packages/eli5/sklearn/explain_weights.py", line 78, in <module>
    from .permutation_importance import PermutationImportance
  File "/usr/local/lib/python3.7/site-packages/eli5/sklearn/permutation_importance.py", line 15, in <module>
    from sklearn.metrics.scorer import check_scoring  # type: ignore
  File "/usr/local/lib/python3.7/site-packages/sklearn/metrics/scorer.py", line 12, in <module>
    _raise_dep_warning_if_not_pytest(deprecated_path, correct_import_path)
  File "/usr/local/lib/python3.7/site-packages/sklearn/utils/deprecation.py", line 143, in _raise_dep_warning_if_not_pytest
    warnings.warn(message, FutureWarning)
FutureWarning: The sklearn.metrics.scorer module is  deprecated in version 0.22 and will be removed in version 0.24. The corresponding classes / functions should instead be imported from sklearn.metrics. Anything that cannot be imported from sklearn.metrics is now part of the private API.

Change as per FutureWarning in sklearn, to allow eli5 to be used with sklearn 0.24 and newer.
@InterferencePattern
Copy link

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).

@icfly2
Copy link
Author

icfly2 commented Dec 9, 2020

Is the upgrade of the sklearn requirement acceptable, given that 0.18 is now 4 years old and 0.24 has a RC out?

@codecov-io
Copy link

codecov-io commented Dec 10, 2020

Codecov Report

Merging #397 (805a3c1) into master (017c738) will decrease coverage by 16.74%.
The diff coverage is 100.00%.

@@             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     
Impacted Files Coverage Δ
eli5/sklearn/permutation_importance.py 93.68% <100.00%> (-6.32%) ⬇️
eli5/keras/gradcam.py 0.00% <0.00%> (-100.00%) ⬇️
eli5/xgboost.py 4.26% <0.00%> (-95.13%) ⬇️
eli5/formatters/image.py 5.47% <0.00%> (-94.53%) ⬇️
eli5/keras/explain_prediction.py 4.81% <0.00%> (-91.57%) ⬇️
eli5/lightgbm.py 4.27% <0.00%> (-90.60%) ⬇️
eli5/catboost.py 11.53% <0.00%> (-88.47%) ⬇️
eli5/_decision_path.py 35.48% <0.00%> (-64.52%) ⬇️
eli5/keras/__init__.py 50.00% <0.00%> (-50.00%) ⬇️
eli5/ipython.py 85.71% <0.00%> (-14.29%) ⬇️
... and 5 more

@lopuhin
Copy link
Contributor

lopuhin commented Dec 11, 2020

Is the upgrade of the sklearn requirement acceptable, given that 0.18 is now 4 years old and 0.24 has a RC out?

I'd say that yes, it is.

Thanks for the PR 👍

@veonua
Copy link

veonua commented Dec 29, 2020

/eli5/sklearn/text.py in

----> 4 from sklearn.feature_extraction.text import VectorizerMixin # type: ignore

should be renamed to _VectorizerMixin

@veonua
Copy link

veonua commented Dec 29, 2020

~/.local/lib/python3.8/site-packages/eli5/sklearn/transform.py
----> 6 from sklearn.feature_selection.base import SelectorMixin # type: ignore

from sklearn.feature_selection import SelectorMixin

@arc12 arc12 mentioned this pull request Jan 6, 2021
@icfly2
Copy link
Author

icfly2 commented Jan 12, 2021

@veonua I'm a bit confused what you are suggesting, master reads:

try:
    from sklearn.feature_extraction.text import _VectorizerMixin as VectorizerMixin
except ImportError:  # Changed in scikit-learn 0.22
    from sklearn.feature_extraction.text import VectorizerMixin

your other comment is also already addressed in this PR

@veonua
Copy link

veonua commented Jan 12, 2021

can you please publish these changes to pip?

@lopuhin
Copy link
Contributor

lopuhin commented Feb 10, 2021

Thanks you! This were merged in eli5-org/eli5#2 and released to PyPI with v0.11

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.

5 participants