-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[python][scikit-learn] add new attribute for used number of features #3129
Conversation
Fixes issue related to scikit-learn/scikit-learn#17353 (see SLEP010 https://scikit-learn-enhancement-proposals.readthedocs.io/en/latest/slep010/proposal.html).
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.
@a-wozniakowski Thanks a lot for your contribution!
To not break the backward compatibility with older scikit-learn versions n_features_
property should be left.
Also, it seems that the current implementation doesn't pass API test and fails at this line:
https://github.com/scikit-learn/scikit-learn/blob/7cc815c21c60dabe8252b30ada77621b88e2136c/sklearn/utils/estimator_checks.py#L2946
I guess that n_features_in_
should be set as an attribute in fit()
method, not as property:
Make sure that n_features_in_ attribute doesn't exist until fit is called
Reverted ```n_features``` property, and inserted the public attribute ```n_features_in_```.
@StrikerRUS thanks!
|
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.
I see, thank you for the explanation!
Now please document new attribute near n_features_
description and I think we are ready to merge.
LightGBM/python-package/lightgbm/sklearn.py
Lines 258 to 259 in bcbf252
n_features_ : int | |
The number of features of fitted model. |
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.
Just one typo:
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.
@a-wozniakowski LGTM! Thank you very much!
…icrosoft#3129) * update number of features attribute Fixes issue related to scikit-learn/scikit-learn#17353 (see SLEP010 https://scikit-learn-enhancement-proposals.readthedocs.io/en/latest/slep010/proposal.html). * Update sklearn.py * set public attribute in fit method Reverted ```n_features``` property, and inserted the public attribute ```n_features_in_```. * Update documentation * Update python-package/lightgbm/sklearn.py Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
…icrosoft#3129) * update number of features attribute Fixes issue related to scikit-learn/scikit-learn#17353 (see SLEP010 https://scikit-learn-enhancement-proposals.readthedocs.io/en/latest/slep010/proposal.html). * Update sklearn.py * set public attribute in fit method Reverted ```n_features``` property, and inserted the public attribute ```n_features_in_```. * Update documentation * Update python-package/lightgbm/sklearn.py Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Fixes issue related to #17353 in scikit-learn (see SLEP010 for further details).