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

Allow for Nans in input of Permutation Importance #381

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

Conversation

Matgrb
Copy link

@Matgrb Matgrb commented May 22, 2020

Closes #262

So far i used force_all_finite=False, but even better would be force_all_finite='allow-nan'. But for that I would also need to increase sklearn version in the requirements to at least 0.20.

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2020

Codecov Report

Merging #381 into master will decrease coverage by 16.74%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #381       +/-   ##
===========================================
- 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

@Matgrb
Copy link
Author

Matgrb commented Jun 9, 2020

This PR is ready for review

Copy link
Contributor

@lopuhin lopuhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Matgrb PR looks great 👍

But for that I would also need to increase sklearn version in the requirements to at least 0.20.

Yes makes sense, it would need to be updated in setup.py and requirements.txt, and also we'd need to adjust or remove py36-legacy in tox.ini

@codecov-io
Copy link

codecov-io commented Nov 11, 2020

Codecov Report

Merging #381 (fc22f65) into master (017c738) will decrease coverage by 16.74%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #381       +/-   ##
===========================================
- 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

@Matgrb
Copy link
Author

Matgrb commented Nov 11, 2020

Hi @lopuhin , sorry for the delay. I have implemented the suggestions from your comment.

@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
4 participants