Skip to content

ENH Adds typing support to LogisticRegression #17799

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

Open
wants to merge 53 commits into
base: main
Choose a base branch
from
Open

ENH Adds typing support to LogisticRegression #17799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
075d7e3
WIP Adds typing support for automatic docstring generation
thomasjpfan Jul 1, 2020
6b35ee1
WIP Temporary fix
thomasjpfan Jul 1, 2020
e158440
WIP Temporary fix
thomasjpfan Jul 1, 2020
4a925e1
WIP Install typing extensions for linting
thomasjpfan Jul 1, 2020
6cb1e79
WIP Try again
thomasjpfan Jul 1, 2020
2930743
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Jul 6, 2020
87f6b07
ENH Adds tests with docstring type generator tool
thomasjpfan Jul 8, 2020
4c47776
STY Lint error
thomasjpfan Jul 8, 2020
7578b63
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Jul 8, 2020
d8c85d0
MNT Adds F821 to ignore flag
thomasjpfan Jul 8, 2020
2e67bc4
BLD Try to fix typing-extensions
thomasjpfan Jul 9, 2020
cf9ff0c
BUG Fix link
thomasjpfan Jul 9, 2020
2535e9f
CI Fixes version?
thomasjpfan Jul 9, 2020
4af558a
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Jul 15, 2020
db3dd08
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Jul 17, 2020
f337c4c
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Jul 29, 2020
d861af4
CLN Update function names
thomasjpfan Aug 3, 2020
292c6ba
WIP Adds tests
thomasjpfan Aug 3, 2020
5484e42
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Aug 3, 2020
751a578
ENH Adds tests for typing
thomasjpfan Aug 4, 2020
fd94e96
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Aug 11, 2020
ddb421a
CLN Imports from typing extensions
thomasjpfan Aug 11, 2020
5999a87
REV
thomasjpfan Aug 11, 2020
0cff327
MNT Moves tests
thomasjpfan Aug 11, 2020
7b6ad2c
MNT Move test to own file
thomasjpfan Aug 11, 2020
e4ea4d8
DOC Adds comment
thomasjpfan Aug 11, 2020
cadb711
ENH Single literal
thomasjpfan Aug 11, 2020
5330c53
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Aug 18, 2020
e790ad8
BUG Fixes bug in python 3.6
thomasjpfan Aug 18, 2020
74c58e1
FIX Adds PolynomialCountSketch to ignore list
thomasjpfan Aug 19, 2020
8e0ee08
CLN Make it easier to merge with future PRs
thomasjpfan Aug 19, 2020
667518e
TST Uses double quotes for strings
thomasjpfan Aug 27, 2020
6ead295
MNT Update license formating for github to recognize
thomasjpfan Aug 28, 2020
c2b9a37
MNT Rename license file
thomasjpfan Aug 28, 2020
b6a5200
Revert "MNT Rename license file"
thomasjpfan Aug 28, 2020
f2e73b4
MNT Less diffs
thomasjpfan Aug 28, 2020
e723ccc
Merge branch 'license_github' into typing
thomasjpfan Aug 28, 2020
c491c98
TST Ignores SequentialFeatureSelector
thomasjpfan Aug 28, 2020
923ac16
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Aug 29, 2020
8d2aa0e
CLN Move Literal into fixes
thomasjpfan Aug 29, 2020
2378769
FIX Fixes spelling of typing_extensions
thomasjpfan Aug 29, 2020
67c4574
MNT Soft dependency on typing_extensions
thomasjpfan Aug 31, 2020
f05e47c
MNT Soft dependency on typing_extensions
thomasjpfan Aug 31, 2020
9f258ad
Merge remote-tracking branch 'upstream/master' into typing
thomasjpfan Jan 4, 2021
301eee1
Merge remote-tracking branch 'upstream/main' into typing
thomasjpfan Jan 24, 2021
e2334ec
ENH Simplify handling of typing
thomasjpfan Jan 25, 2021
7bbcec7
Merge remote-tracking branch 'upstream/main' into typing
thomasjpfan Jan 25, 2021
459c0bf
ENH Better diffs
thomasjpfan Jan 25, 2021
39d3fea
ENH Simplify to only hyper-parameters
thomasjpfan Jan 25, 2021
b43dd9c
ENH Adds cvsplit
thomasjpfan Jan 25, 2021
63c0591
Merge remote-tracking branch 'upstream/main' into typing
thomasjpfan Jun 11, 2021
09cad30
CLN Smaller diff
thomasjpfan Jun 11, 2021
64079ea
Merge remote-tracking branch 'upstream/main' into typing
thomasjpfan Apr 24, 2022
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
4 changes: 4 additions & 0 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ per-file-ignores =
ignore_missing_imports = True
allow_redefinition = True

[mypy-sklearn.externals.*]
check_untyped_defs=False
ignore_errors=True

[check-manifest]
# ignore files missing in VCS
ignore =
Expand Down
37 changes: 22 additions & 15 deletions sklearn/linear_model/_logistic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,16 @@
# Lars Buitinck
# Simon Wu <s8wu@uwaterloo.ca>
# Arthur Mensch <arthur.mensch@m4x.org
from __future__ import annotations
import typing

if typing.TYPE_CHECKING:
from typing_extensions import Literal

import numbers
import warnings
from typing import Union
from typing import Optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why these are all on a separate line?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a personal style choice so diffs are easier to look at. I also like:

from typing import (
	Union,
	Optional,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

True as well.

Unrelated: Does sklearn uses black?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are slowly working our way on deciding this: #11336

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is relevant here: #11336 (comment)

If we go with adding typing information, then black is reasonable for its style choices.

Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from typing import Union
from typing import Optional
from typing import Mapping


import numpy as np
from scipy import optimize
Expand Down Expand Up @@ -1028,22 +1035,22 @@ class LogisticRegression(LinearClassifierMixin, SparseCoefMixin, BaseEstimator):

def __init__(
self,
penalty="l2",
penalty: Literal["l1", "l2", "elasticnet", "none"] = "l2",
*,
dual=False,
tol=1e-4,
C=1.0,
fit_intercept=True,
intercept_scaling=1,
class_weight=None,
random_state=None,
solver="lbfgs",
max_iter=100,
multi_class="auto",
verbose=0,
warm_start=False,
n_jobs=None,
l1_ratio=None,
dual: bool = False,
tol: float = 1e-4,
C: float = 1.0,
fit_intercept: bool = True,
intercept_scaling: float = 1,
class_weight: Union[dict, Literal["balanced"]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

As annotations are not evaluated with from __future__ import annotations, we can use directly the future | syntax inside them. It is already supported by Mypy.

Also, don't use the dict type unless you want to allow inserting operations, and specify the known types for keys and values.

In addition, the allowed value None was missing.

Suggested change
class_weight: Union[dict, Literal["balanced"]] = None,
class_weight: Mapping[str | float, float] | Literal["balanced"] | None = None,

random_state: Union[int, np.random.RandomState, None] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be useful to define a RandomLike type.

Suggested change
random_state: Union[int, np.random.RandomState, None] = None,
random_state: int | np.random.RandomState | None = None,

solver: Literal["newton-cg", "lbfgs", "liblinear", "sag", "saga"] = "lbfgs",
max_iter: int = 100,
multi_class: Literal["auto", "ovr", "multinomial"] = "auto",
verbose: int = 0,
warm_start: bool = False,
n_jobs: Optional[int] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
n_jobs: Optional[int] = None,
n_jobs: int | None = None,

l1_ratio: Optional[float] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
l1_ratio: Optional[float] = None,
l1_ratio: float | None = None,

):

self.penalty = penalty
Expand Down