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 for eval_at and basic #5674

Merged
merged 3 commits into from
Jan 13, 2023
Merged

[python-package] fix mypy errors for eval_at and basic #5674

merged 3 commits into from
Jan 13, 2023

Conversation

IdoKendo
Copy link
Contributor

Contributes to #3867.
Moved from #5672.

Fixes mypy errors regarding eval_at incompatibility:

Argument "eval_at" to "_train" has incompatible type "Optional[Iterable[int]]"; expected "Union[List[int], Tuple[int], None]"  [arg-type]

and the following two errors:

python-package\lightgbm\engine.py:169: error: Argument 1 to "set_feature_name" of "Dataset" has incompatible type "Union[List[str], str]"; expected "List[str]"  [arg-type]
python-package\lightgbm\engine.py:170: error: Argument 1 to "set_categorical_feature" of "Dataset" has incompatible type "Union[List[str], List[int], str]"; expected "Union[List[int], List[str]]"  [arg-type]

Notes for Reviewers

This was tested by running mypy as documented in #3867.

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

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this! I left one question for your consideration.

@@ -407,7 +407,7 @@ def _train(
eval_init_score: Optional[List[_DaskCollection]] = None,
eval_group: Optional[List[_DaskVectorLike]] = None,
eval_metric: Optional[_LGBM_ScikitEvalMetricType] = None,
eval_at: Optional[Union[List[int], Tuple[int]]] = None,
eval_at: Union[List[int], Tuple[int, ...], None] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is Union[something, something_else, None] preferable to Optional[Union[something, something_else]]? If that's just a personal style preference thing, please revert that part of this change. I personally prefer the Optional[] form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, no problem, changed it to Optional[] form.

@IdoKendo IdoKendo requested review from jameslamb and removed request for jmoralez, shiyu1994 and StrikerRUS January 12, 2023 15:54
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks so much, changes look great to me! I just merged latest master into this, will merge this once CI passes.

@jameslamb jameslamb merged commit 5df7d51 into microsoft:master Jan 13, 2023
@IdoKendo IdoKendo deleted the python-package/fix-mypy-errors-basic-and-eval-at branch March 9, 2023 15:20
@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 15, 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