search

TeamHG-Memex / eli5

A library for debugging/inspecting machine learning classifiers and explaining their predictions
http://eli5.readthedocs.io
MIT License
2.75k stars 331 forks source link

Catboost support fixes #276 explain weights #303

Closed AshwinB-hat closed 5 years ago

AshwinB-hat commented 5 years ago

explain_weights for catboost is added with this commit along with test scenarios.

lopuhin commented 5 years ago

Thanks for PR @AshwinB-hat , it's great that you added tests and a detailed docstring right away πŸ‘

I noticed that you also added a lot of files from running catboost (such as files under "catboost_cached_datasets"), probably you didn't mean to commit them? I think it would be nice to exclude them from the PR.

Also a note regarding the tests, there are two things that would need to be adjusted. Tests are run in different environments, you can run them with tox (see here https://eli5.readthedocs.io/en/latest/contribute.html). Right now there are two issues:

Will do a code review a bit later, thanks for the PR πŸ‘

AshwinB-hat commented 5 years ago

@lopuhin Yes i will revert the commit and look into lightgbm

codecov-io commented 5 years ago

Codecov Report

Merging #303 into master will increase coverage by 0.02%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
+ Coverage   97.16%   97.19%   +0.02%     
==========================================
  Files          44       45       +1     
  Lines        2862     2892      +30     
  Branches      544      547       +3     
==========================================
+ Hits         2781     2811      +30     
  Misses         43       43              
  Partials       38       38
Impacted Files Coverage Ξ”
eli5/__init__.py 80% <ΓΈ> (+2.22%) :arrow_up:
eli5/catboost.py 100% <100%> (ΓΈ)
AshwinB-hat commented 5 years ago

@lopuhin I have made the mentioned changes. Although one environment test is failing.

AshwinB-hat commented 5 years ago

@lopuhin I have updated the PR. Please let me know if any other changes required.

lopuhin commented 5 years ago

Thanks @AshwinB-hat , PR looks great πŸ‘ I'll check if there is anything that needs to be changed in the docs.

lopuhin commented 5 years ago

Hi @AshwinB-hat I checked the docs, there are a few issues, but IMO it's also fine to merge this PR and add the docs in a later PR. Here are the docs issues (btw you can check them locally if you go to ./docs/ folder, install requirements and run make html):

@AshwinB-hat please tell if you'd like to finish the docs in this PR (this can be done later as well), or proceed with the merge?

AshwinB-hat commented 5 years ago

@lopuhin I prefer I make a different PR for the Catboost docs.

Currently I am investigating the SHAP paper and writing my proposal which I might run by @lopuhin @kmike in a day or two for feedback.

I will look into docs following the submission of my GSOC proposal and start working on the catboost explain_prediction as well.

That is my plan of action as of now.

lopuhin commented 5 years ago

@AshwinB-hat great, that's a solid plan πŸ‘

@kmike would you like to check this PR?

kmike commented 5 years ago

Hey! Could you please add docs in the same pull request, to make it complete? I think that's good to have all features tested and documented.

AshwinB-hat commented 5 years ago

@kmike Yes, will complete the documentation.

AshwinB-hat commented 5 years ago

@kmike @lopuhin I have added the docs and resolved the warnings. They were arising due to the tutorials not being added in toc tree.

I am done with my exams and other commitments hence the delay. I would be interested on working on the next step that is the prediction.

Also Let me know if any improvement or changes.

AshwinB-hat commented 5 years ago

hi @lopuhin, You must be flooded with notifications. Just making sure you have seen the above comment. If there are more changes to be done before the merge, please let me know. It will be mean a lot to me if this change was merged.. as it is my first contribution.

lopuhin commented 5 years ago

Hi @AshwinB-hat , many thanks for adding the docs and resolving comments, I gave a quick look and it looks great, will check in more detail tomorrow morning.

ivanprado commented 5 years ago

Hi @AshwinB-hat! I'll have a look at the PR so that we can have it merged soon :-).

Could you please update the README file to include the new feature? (https://github.com/AshwinB-hat/eli5/blob/master/README.rst)

AshwinB-hat commented 5 years ago

@ivanprado Yes! will do it right away!

ivanprado commented 5 years ago

Hi @AshwinB-hat !

I see some formatting issues in the documentation. That's what I see: image Could you please have a look? The best way is by building documentation by yourself. Here you have the instructions: https://github.com/TeamHG-Memex/eli5/tree/master/docs

ivanprado commented 5 years ago

https://github.com/TeamHG-Memex/eli5/pull/313 has been merged in master, and that should fix the build. Could you please merge back master with your fork? That should make Travis happy :-)

AshwinB-hat commented 5 years ago

@ivanprado I made the suggested changes and some. Looks fine on the local build (thanks for the tip). Travis seems relatively happier since before. :)

ivanprado commented 5 years ago

Nice @AshwinB-hat, I'll have a look to the changes! I see the following error in Travis: /home/travis/build/TeamHG-Memex/eli5/docs/source/autodocs/catboost.rst:4:Inline literal start-string without end-string. Could you please have a look?

ivanprado commented 5 years ago

Ok, I had a look, we are very close! Remaining:

AshwinB-hat commented 5 years ago

@ivanprado Guess its good news!

ivanprado commented 5 years ago

@AshwinB-hat congratulations!. Your first contribution is now in master :-)

kmike commented 5 years ago

πŸŽ‰

AshwinB-hat commented 5 years ago

@lopuhin @ivanprado @kmike Awesome. Thanks for the guidance leading up till this. :D