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

add lightgbm.booster support #270

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

Conversation

qh582
Copy link

@qh582 qh582 commented May 29, 2018

mainly followed the logic of xgboost.booster support

qh582 added 4 commits May 29, 2018 11:34
mainly followed the logic of xgboost.booster support
lgb2.1.0 to lgb  2.1.1
update to lgb2.1.1
@codecov-io
Copy link

codecov-io commented May 29, 2018

Codecov Report

Merging #270 into master will decrease coverage by <.01%.
The diff coverage is 97.91%.

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   97.22%   97.22%   -0.01%     
==========================================
  Files          44       44              
  Lines        2815     2845      +30     
  Branches      536      545       +9     
==========================================
+ Hits         2737     2766      +29     
  Misses         41       41              
- Partials       37       38       +1
Impacted Files Coverage Δ
eli5/lightgbm.py 95.23% <97.91%> (+0.36%) ⬆️

eli5/lightgbm.py Outdated
proba=proba,
get_score_weights=get_score_weights,
)


def _check_booster_args(lgb, is_regression=None):
# type: (Any, bool) -> Tuple[Booster, bool]
Copy link
Contributor

@lopuhin lopuhin May 29, 2018

Choose a reason for hiding this comment

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

I hope something like # type: (Any, bool) -> Tuple[lightgbm.Booster, bool] and adding Tuple and Any to typing imports at the top should fix the build (https://travis-ci.org/TeamHG-Memex/eli5/jobs/385021206#L764-L766). To check locally, you can try installing mypy==0.550 and running mypy --check-untyped-defs eli5

@lopuhin
Copy link
Contributor

lopuhin commented May 29, 2018

Thanks for the PR @qh582 , at first sight it looks good 👍
I left a comment on how to fix the build. It would be also great if you could add tests for lightgbm.booster support, so that we don't break it in the future.

@qh582
Copy link
Author

qh582 commented May 30, 2018

@lopuhin Thanks for review and suggestion.

solved it.


I am doing the explain_prediction for regression part test, and I got 'y=y' in stead of 'y' as shown in the screenshot:
image
This seems ok for me, but I found eli5 using assert '<b>y</b>' in strip_blanks(expl_html) in tests.test_sklearn_explain_prediction.assert_trained_linear_regression_explained which point that it shoud be 'y' in my case. I am wondering if I missed the point here.

@qh582
Copy link
Author

qh582 commented May 31, 2018

@lopuhin could you please check it?

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.

Looks great, thanks @qh582 ! I'm +1 to merge this, @kmike do you want to have a look?

@kmike
Copy link
Contributor

kmike commented May 31, 2018

qh582 added 3 commits May 31, 2018 19:56
* Update overview.rst

* Update README.rst

* Update lightgbm.rst
* Update overview.rst

* Update README.rst

* Update lightgbm.rst
@qh582
Copy link
Author

qh582 commented May 31, 2018

@kmike I think this is ready, please check :)

("objective" starts with "reg")
and False for a classification problem.
If not set, regression is assumed for a single target estimator
and proba will not be shown unless the ``target_names`` is defined as a list with length of two.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to contradict a note above (line 25, "target_names is ignored") - does the note about target_names apply only to classifier/regressor, but not for Booster?

Copy link
Author

Choose a reason for hiding this comment

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

for a single target estimater, booster cannot recognize wheather it is a regression problem or not. We assume it is regression in default, unless users set it as a classification problem by assigning 'target names' input [0,1] etc. Only in this case 'target names' is used. Should I remove " target names is ignored" to prevent confusion?

Copy link
Contributor

Choose a reason for hiding this comment

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

target names is still ignored for classifier/regressor, right? I think it makes sense to clarify it - just say that it is ignored for LGBMClassifier / LGBMRegressor, but used for lightgbm.Booster.

"is defined as a list with length of two" - sohuld it be 2 elements, or 2+ is also supported?

Copy link
Author

@qh582 qh582 May 31, 2018

Choose a reason for hiding this comment

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

Well, for single target booster, only 1+ elements target_name point booster to classification. Otherwise, this booster is regression . there is risk user assign wrong number of elements like 3, but not sure how eli5 will raise error. 2+ should not support here.

and :func:`eli5.explain_prediction` for ``lightgbm.LGBMClassifer``
and ``lightgbm.LGBMRegressor`` estimators.
and :func:`eli5.explain_prediction` for ``lightgbm.LGBMClassifer``, ``lightgbm.LGBMRegressor`` and ``lightgbm.Booster`` estimators. It is tested against LightGBM
master branch.
Copy link
Contributor

Choose a reason for hiding this comment

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

"It is tested against LightGBM master branch." is removed in 63e9918, sorry for the confusion. It shouldn't be re-added.

Copy link
Author

Choose a reason for hiding this comment

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

got it, I will remove that line in next commit

@qh582
Copy link
Author

qh582 commented May 31, 2018

@kmike Hope the new commit help. My previous piece of code is not strict enough.

@WestXu
Copy link

WestXu commented Aug 8, 2018

It's been two month, could someone tell me why this merge hasn't been closed already? I really need this.

@lopuhin
Copy link
Contributor

lopuhin commented Nov 14, 2018

I think PR is in great shape and we just need to check it again and merge it, hope to find time for it soonish, sorry for delay.


.. _LightGBM: https://github.com/Microsoft/LightGBM

:func:`eli5.explain_weights` uses feature importances. Additional
arguments for LGBMClassifier and LGBMClassifier:
arguments for LGBMClassifier , LGBMClassifier and lightgbm.Booster:
Copy link
Contributor

Choose a reason for hiding this comment

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

a nitpick: extra whitespace before comma

@@ -22,7 +21,7 @@ arguments for LGBMClassifier and LGBMClassifier:
- 'weight' - the same as 'split', for better compatibility with
:ref:`library-xgboost`.

``target_names`` and ``target`` arguments are ignored.
``target_names`` arguement is ignored for ``lightgbm.LGBMClassifer`` / ``lightgbm.LGBMRegressor``, but used for ``lightgbm.Booster``. ``target`` argument is ignored.
Copy link
Contributor

@kmike kmike Nov 18, 2018

Choose a reason for hiding this comment

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

Suggested change
``target_names`` arguement is ignored for ``lightgbm.LGBMClassifer`` / ``lightgbm.LGBMRegressor``, but used for ``lightgbm.Booster``. ``target`` argument is ignored.
``target_names`` argument is ignored for ``lightgbm.LGBMClassifer`` / ``lightgbm.LGBMRegressor``, but used for ``lightgbm.Booster``. ``targets`` argument is ignored.


.. _LightGBM: https://github.com/Microsoft/LightGBM

:func:`eli5.explain_weights` uses feature importances. Additional
arguments for LGBMClassifier and LGBMClassifier:
arguments for LGBMClassifier , LGBMClassifier and lightgbm.Booster:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
arguments for LGBMClassifier , LGBMClassifier and lightgbm.Booster:
arguments for LGBMClassifier, LGBMClassifier and lightgbm.Booster:

@@ -22,7 +21,7 @@ arguments for LGBMClassifier and LGBMClassifier:
- 'weight' - the same as 'split', for better compatibility with
:ref:`library-xgboost`.

``target_names`` and ``target`` arguments are ignored.
``target_names`` arguement is ignored for ``lightgbm.LGBMClassifer`` / ``lightgbm.LGBMRegressor``, but used for ``lightgbm.Booster``. ``target`` argument is ignored.

.. note::
Top-level :func:`eli5.explain_weights` calls are dispatched
Copy link
Contributor

Choose a reason for hiding this comment

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

I think lightgbm.Booster should be mentioned here as well.

@@ -50,6 +49,14 @@ for ``lightgbm.LGBMClassifer`` and ``lightgbm.LGBMRegressor``:
estimator. Set it to True if you're passing ``vec``,
but ``doc`` is already vectorized.

``lightgbm.Booster`` estimator accepts one more optional argument:

* ``is_regression`` - True if solving a regression problem
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this parameter supported? It is not an argument of explain_prediction_lightgbm.

@@ -31,7 +31,7 @@ following machine learning frameworks and packages:
of XGBClassifier, XGBRegressor and xgboost.Booster.

* :ref:`library-lightgbm` - show feature importances and explain predictions
of LGBMClassifier and LGBMRegressor.
of LGBMClassifier , LGBMRegressor and lightgbm.Booster.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
of LGBMClassifier , LGBMRegressor and lightgbm.Booster.
of LGBMClassifier, LGBMRegressor and lightgbm.Booster.

``target_names`` and ``targets`` parameters are ignored.
``target_names`` arguement is ignored for ``lightgbm.LGBMClassifer`` / ``lightgbm.LGBMRegressor``,
but used for ``lightgbm.Booster``.
``target`` argument is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``target`` argument is ignored.
``targets`` argument is ignored.

@Dronablo
Copy link

Any plans about merging it, @lopuhin?

@lopuhin
Copy link
Contributor

lopuhin commented Feb 10, 2021

Thanks you! This were merged in eli5-org/eli5#7 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.

6 participants