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

[python-package] fix mypy errors in sklearn.py #4837

Merged
merged 4 commits into from
Dec 2, 2021
Merged

Conversation

jameslamb
Copy link
Collaborator

Contributes to #3867.

This PR fixes mypy errors in sklearn.py.

python-package/lightgbm/sklearn.py:532: error: Need type annotation for "_other_params" (hint: "_other_params: Dict[, ] = ...")
python-package/lightgbm/sklearn.py:879: error: "None" has no attribute "current_iteration"
python-package/lightgbm/sklearn.py:890: error: "None" has no attribute "current_iteration"
python-package/lightgbm/sklearn.py:944: error: Item "None" of "Optional[str]" has no attribute "replace"
python-package/lightgbm/sklearn.py:1011: error: Item "None" of "Optional[str]" has no attribute "replace"
python-package/lightgbm/sklearn.py:1110: error: Item "None" of "Optional[str]" has no attribute "replace"

Confirmed that this suppresses these warnings by running the following.

mypy \
    --exclude='python-package/compile/|python-package/build' \
    --ignore-missing-imports \
    python-package/

@@ -1008,7 +1008,7 @@ def fit(self, X, y,
callbacks=callbacks, init_model=init_model)
return self

_base_doc = LGBMModel.fit.__doc__.replace("self : LGBMModel", "self : LGBMClassifier")
_base_doc = LGBMModel.fit.__doc__.replace("self : LGBMModel", "self : LGBMClassifier") # type: ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

warnings about .__doc__ being Optional[str] could be avoided by changing this to something like getattr(LGBMModel.fit, "__doc__", ""), but I think this is preferable, so that .replace() raises an exception in CI if something is accidentally changed that results in the docs being empty for a method.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Type hints and #ignore directives look OK to me, but I disagree with actual code complication for the sake of removing mypy false alarm.

@jameslamb jameslamb requested a review from StrikerRUS December 2, 2021 01:43
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants