-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
FIX changed_only=True with kwargs parameters #17205
Conversation
sklearn/utils/tests/test_pprint.py
Outdated
from lightgbm import LGBMClassifier # noqa | ||
|
||
# metric is part of **kwargs | ||
est = LGBMClassifier(metric='auc', max_depth=10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should test something=None for non-regression?
sklearn/utils/tests/test_pprint.py
Outdated
# https://github.com/scikit-learn/scikit-learn/issues/17206 | ||
|
||
pytest.importorskip("lightgbm") | ||
from lightgbm import LGBMClassifier # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to implement something with def __init__(**kw)
here??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, LightGBM does weird stuff with get_params. But this is a good fix.
Please update what's new for 0.23.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the fix.
Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
Thanks for the reviews! @adrinjalali is there a PR/issue to tag for inclusion into 0.23.1? |
No I just created the milestone and will include them next week in a PR. Putting in the milestone should be enough :) |
Now the doctests work in versions where scikit-learn/scikit-learn#17205 is merged.
Now the doctests work in versions where scikit-learn/scikit-learn#17205 is merged.
Fixes #17206
@thomasjpfan @adrinjalali @rth @jnothman