-
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] Migrate to f-strings in python-package/lightgbm/sklearn.py #4188
Conversation
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.
Thanks for picking this up! Please see my comments below.
I also made one small change to the PR. I added a note to the description mentioning #4136, so that there is a link connecting this PR to a previous issue and vice versa. We use that linking feature heavily in this project, to keep track of the connections between different discussions.
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
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.
looks good, thank you!
"input n_features is %s " | ||
% (self._n_features, n_features)) | ||
f"match the input. Model n_features_ is {self._n_features} and " | ||
f"input n_features is {n_features} ") | ||
return self._Booster.predict(X, raw_score=raw_score, start_iteration=start_iteration, num_iteration=num_iteration, | ||
pred_leaf=pred_leaf, pred_contrib=pred_contrib, **kwargs) | ||
|
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.
Please update the following lines within this PR as well
LightGBM/python-package/lightgbm/sklearn.py
Lines 535 to 536 in 211ef78
if hasattr(self, '_' + key): | |
setattr(self, '_' + key, value) |
LightGBM/python-package/lightgbm/sklearn.py
Lines 997 to 998 in 211ef78
+ ' ' * 12 + 'The evaluation positions of the specified metric.\n' | |
+ ' ' * 8 + _early_stop + _after_early_stop) |
' ' * x
)LightGBM/python-package/lightgbm/sklearn.py
Line 655 in 211ef78
raise TypeError('{} should be dict or list'.format(name)) |
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.
@StrikerRUS do I only change the ' '*x
part or do I also modify the concatenation taking place there into an f-string as below?
space = " " fit.__doc__ = (f"{_before_early_stop}" \ f"eval_at : iterable of int, optional (default=(1, 2, 3, 4, 5))\n" \ f"{space*12} The evaluation positions of the specified metric.\n" \ f"{space*8} {_early_stop} {_after_early_stop}")
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.
Originally I thought about only ' '*x
part, but just saw that @jameslamb would prefer modifying concatenation as well: #4144 (comment). So, please modify it with concatenation.
BTW, there are some other places where concatenation can be eliminated with f-strings:
LightGBM/python-package/lightgbm/sklearn.py
Lines 705 to 711 in 211ef78
fit.__doc__ = _lgbmmodel_doc_fit.format( | |
X_shape="array-like or sparse matrix of shape = [n_samples, n_features]", | |
y_shape="array-like of shape = [n_samples]", | |
sample_weight_shape="array-like of shape = [n_samples] or None, optional (default=None)", | |
init_score_shape="array-like of shape = [n_samples] or None, optional (default=None)", | |
group_shape="array-like or None, optional (default=None)" | |
) + "\n\n" + _lgbmmodel_doc_custom_eval_note |
LightGBM/python-package/lightgbm/sklearn.py
Lines 825 to 831 in 211ef78
_base_doc = LGBMModel.fit.__doc__ | |
_base_doc = (_base_doc[:_base_doc.find('group :')] # type: ignore | |
+ _base_doc[_base_doc.find('eval_set :'):]) # type: ignore | |
_base_doc = (_base_doc[:_base_doc.find('eval_class_weight :')] | |
+ _base_doc[_base_doc.find('eval_init_score :'):]) | |
fit.__doc__ = (_base_doc[:_base_doc.find('eval_group :')] | |
+ _base_doc[_base_doc.find('eval_metric :'):]) |
LightGBM/python-package/lightgbm/sklearn.py
Lines 898 to 902 in 211ef78
_base_doc = LGBMModel.fit.__doc__ | |
_base_doc = (_base_doc[:_base_doc.find('group :')] # type: ignore | |
+ _base_doc[_base_doc.find('eval_set :'):]) # type: ignore | |
fit.__doc__ = (_base_doc[:_base_doc.find('eval_group :')] | |
+ _base_doc[_base_doc.find('eval_metric :'):]) |
LightGBM/python-package/lightgbm/sklearn.py
Lines 990 to 992 in 211ef78
_base_doc = LGBMModel.fit.__doc__ | |
fit.__doc__ = (_base_doc[:_base_doc.find('eval_class_weight :')] # type: ignore | |
+ _base_doc[_base_doc.find('eval_init_score :'):]) # type: ignore |
But I don't think we should replace concatenation with f-strings there because concatenation is more readable in those cases in my opinion. @jameslamb WDYT?
Also, please check the following comment about padding #4144 (comment).
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.
But I don't think we should replace concatenation with f-strings there because concatenation is more readable in those cases in my opinion
Yes I agree with you.
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.
So I infer that no concatenations be migrated then.
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.
yes, cases like the ones @StrikerRUS mentioned (where .format()
was used to replace a shared template like _lgbmmodel_doc_fit
or where there are just variables being added to each other but not string literals) do not need to be changed for this pull request
Co-authored-by: Nikita Titov <[email protected]>
…ghtGBM into fstrings-patch-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.
Please see my new round of comments, and let me know if you have any questions! If it seems like we're giving you confusing or contradictory suggestions I apologize.
Co-authored-by: James Lamb <[email protected]>
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.
looks great, thank you!
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!
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. |
Switches some string formatting in
sklearn.py
to use f-strings, as part of #4136.