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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Apr 19, 2021
Merged

[python] Migrate to f-strings in python-package/lightgbm/sklearn.py #4188

Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
76c51de
Migrate to f-strings in python-package/lightgbm/sklearn.py
Apr 16, 2021
14f2b7e
Apply suggestions from code review
akshitadixit Apr 16, 2021
dadcd8a
Update python-package/lightgbm/sklearn.py
akshitadixit Apr 17, 2021
66ea376
Apply suggestions from code review
akshitadixit Apr 17, 2021
3a449e5
Add suggestions from code review
Apr 18, 2021
3795edc
Merge branch 'fstrings-patch-1' of https://github.com/akshitadixit/Li…
Apr 18, 2021
7c5836c
resolve conflicts
Apr 18, 2021
70c677d
Apply suggestions from code review
akshitadixit Apr 19, 2021
968825b
Update sklearn.py
akshitadixit Apr 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions python-package/lightgbm/sklearn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def __call__(self, preds, dataset):
elif argc == 3:
grad, hess = self.func(labels, preds, dataset.get_group())
else:
raise TypeError("Self-defined objective function should have 2 or 3 arguments, got %d" % argc)
raise TypeError(f"Self-defined objective function should have 2 or 3 arguments, got {argc}")
"""weighted for objective"""
weight = dataset.get_weight()
if weight is not None:
Expand Down Expand Up @@ -171,7 +171,7 @@ def __call__(self, preds, dataset):
elif argc == 4:
return self.func(labels, preds, dataset.get_weight(), dataset.get_group())
else:
raise TypeError("Self-defined eval function should have 2, 3 or 4 arguments, got %d" % argc)
raise TypeError(f"Self-defined eval function should have 2, 3 or 4 arguments, got {argc}")


# documentation templates for LGBMModel methods are shared between the classes in
Expand Down Expand Up @@ -532,8 +532,8 @@ def set_params(self, **params):
"""
for key, value in params.items():
setattr(self, key, value)
if hasattr(self, '_' + key):
setattr(self, '_' + key, value)
if hasattr(self, f"_{key}"):
setattr(self, f"_{key}", value)
self._other_params[key] = value
return self

Expand Down Expand Up @@ -652,7 +652,7 @@ def _get_meta_data(collection, name, i):
elif isinstance(collection, dict):
return collection.get(i, None)
else:
raise TypeError('{} should be dict or list'.format(name))
raise TypeError(f"{name} should be dict or list")

if isinstance(eval_set, tuple):
eval_set = [eval_set]
Expand Down Expand Up @@ -720,9 +720,8 @@ def predict(self, X, raw_score=False, start_iteration=0, num_iteration=None,
n_features = X.shape[1]
if self._n_features != n_features:
raise ValueError("Number of features of the model must "
"match the input. Model n_features_ is %s and "
"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)

Copy link
Collaborator

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)
(part about ' ' * x)

LightGBM/python-package/lightgbm/sklearn.py

Line 655 in 211ef78

raise TypeError('{} should be dict or list'.format(name))

Copy link
Contributor Author

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}")

Copy link
Collaborator

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).

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Expand Down Expand Up @@ -993,6 +992,6 @@ def fit(self, X, y,
_base_doc = fit.__doc__
_before_early_stop, _early_stop, _after_early_stop = _base_doc.partition('early_stopping_rounds :')
fit.__doc__ = (_before_early_stop
+ 'eval_at : iterable of int, optional (default=(1, 2, 3, 4, 5))\n'
+ ' ' * 12 + 'The evaluation positions of the specified metric.\n'
+ ' ' * 8 + _early_stop + _after_early_stop)
"eval_at : iterable of int, optional (default=(1, 2, 3, 4, 5))\n"
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
f"{' ':12}The evaluation positions of the specified metric.\n"
f"{' ':8}{_early_stop}{_after_early_stop}")