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] add more type hints on LGBMModel methods #5239

Merged
merged 1 commit into from
May 31, 2022

Conversation

jameslamb
Copy link
Collaborator

Contributes to #3756.

Adds type hints on some methods in lightgbm.sklearn.LGBMModel that don't currently have them.

@jmoralez
Copy link
Collaborator

Most of these changes raise errors on mypy (logs):

python-package/lightgbm/sklearn.py:830: error: Incompatible return value type (got "None", expected "int")
python-package/lightgbm/sklearn.py:837: error: Incompatible return value type (got "None", expected "int")
python-package/lightgbm/sklearn.py:851: error: Incompatible return value type (got "None", expected "int")
python-package/lightgbm/sklearn.py:858: error: Incompatible return value type (got "Union[str, Callable[[Any, Any], Tuple[Any, Any]], Callable[[Any, Any, Any], Tuple[Any, Any]], None]", expected "Union[str, Union[Callable[[Any, Any], Tuple[Any, Any]], Callable[[Any, Any, Any], Tuple[Any, Any]]]]")
python-package/lightgbm/sklearn.py:1095: error: Incompatible return value type (got "None", expected "int")

Although I believe the if statement is enough to check that most of them aren't None, I'm not sure if mypy isn't able to see it.

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented May 28, 2022

I remember the following discussion about these mypy false alarms: #4837 (review), #4837 (comment).

@jameslamb
Copy link
Collaborator Author

Thanks for finding those links @StrikerRUS ! I remembered those conversations before, and am ok with the conclusion we reached there, that # type: ignore comments could be used to handle these cases where we "just know" that an attribute like n_classes_ will always either be an integer (if model has been fitted) or raise an exception (if not fitted).

I didn't add such comments in this PR because I just wanted to focus on getting the type hints correct, and wanted to leave mypy warnings for separate PRs targeting #3867.

@jameslamb jameslamb merged commit 4971a06 into master May 31, 2022
@jameslamb jameslamb deleted the more-sklearn-type-hints branch May 31, 2022 03:50
@StrikerRUS
Copy link
Collaborator

I didn't add such comments in this PR because I just wanted to focus on getting the type hints correct, and wanted to leave mypy warnings for separate PRs targeting #3867.

Great, totally agree!

@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@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 19, 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.

3 participants