Skip to content

TYP: Improve clip() return and argument typing. #40860

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

Merged
merged 4 commits into from
Apr 11, 2021
Merged

TYP: Improve clip() return and argument typing. #40860

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f613b1
Improve clip() typing.
DriesSchaumont Apr 9, 2021
fb851fd
Update typing.
DriesSchaumont Apr 9, 2021
ace7fea
More clip overloads.
DriesSchaumont Apr 10, 2021
c89e308
Add missing axis types
DriesSchaumont Apr 10, 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
112 changes: 110 additions & 2 deletions pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7367,16 +7367,124 @@ def _clip_with_one_bound(self, threshold, method, axis, inplace):
threshold = align_method_FRAME(self, threshold, axis, flex=None)[1]
return self.where(subset, threshold, axis=axis, inplace=inplace)

@overload
def clip(
self: FrameOrSeries,
lower=...,
upper=...,
axis: Axis | None = ...,
Copy link
Member

Choose a reason for hiding this comment

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

users of the pandas api should not pass None.

axis: int or str axis name, optional

for the default (should be listed in the docstring instead of optional - not for this PR though), the argument should not be passed.

@MarcoGorelli what are you thoughts about not including None in the overloads and also now that we are using PEP 604, removing no_implicit_optional = True from setup.cfg and excluding None actual function signatures too?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @simonjayhawkins

+1 on removing no_implicit_optional

Not sure what you mean about not including None in overloads - if axis=None is passed internally, then won't it need to be in one of the overloads for type checking to work?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the question is (in general) should None ever be passed internally or should kwargs always be (in general) validated at the top level.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC we have had issues reported where None is passed by users and when we change defaults (I think bool True/False in a specific case I remember recently) this is results in a perceived regression. We also sometime use lib.nodefault to detect if a kwarg has been specified. So I was thinking to that discouraging users passing None could be worthwhile.

Copy link
Member

Choose a reason for hiding this comment

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

@simonjayhawkins I looked into the no_implicit_optional = True, but it seems it's only possible to omit if you only have two possible types.

e.g. the following works:

from __future__ import annotations
from typing import Tuple

def foo(
    a: str | int | None  = None,
    b: str = None,    
): ...

but the following doesn't:

from __future__ import annotations
from typing import Tuple

def foo(
    a: str | int  = None,
    b: str = None,    
): ...

, we get

t.py:5: error: Incompatible default for argument "a" (default has type "None", argument has type "Union[str, int]")  [assignment]
Found 1 error in 1 file (checked 1 source file)

So I think it's cleaerer to keep no_implicit_optional

inplace: Literal[False] = ...,
*args,
**kwargs,
) -> FrameOrSeries:
...

@overload
def clip(
self: FrameOrSeries,
lower,
*,
axis: Axis | None,
inplace: Literal[True],
**kwargs,
) -> None:
...

@overload
def clip(
self: FrameOrSeries,
lower,
*,
inplace: Literal[True],
**kwargs,
) -> None:
...

@overload
def clip(
self: FrameOrSeries,
*,
upper,
axis: Axis | None,
inplace: Literal[True],
**kwargs,
) -> None:
...

@overload
def clip(
self: FrameOrSeries,
*,
upper,
inplace: Literal[True],
**kwargs,
) -> None:
...

@overload
def clip(
self: FrameOrSeries,
*,
axis: Axis | None,
inplace: Literal[True],
**kwargs,
) -> None:
...

@overload
def clip(
self: FrameOrSeries,
lower,
upper,
axis: Axis | None,
inplace: Literal[True],
*args,
**kwargs,
) -> None:
...

@overload
def clip(
self: FrameOrSeries,
lower,
upper,
*,
inplace: Literal[True],
**kwargs,
) -> None:
...

@overload
def clip(
self: FrameOrSeries,
*,
inplace: Literal[True],
**kwargs,
) -> None:
...

@overload
def clip(
self: FrameOrSeries,
lower=...,
upper=...,
axis: Axis | None = ...,
inplace: bool_t = ...,
*args,
**kwargs,
) -> FrameOrSeries | None:
...

@final
def clip(
self: FrameOrSeries,
lower=None,
upper=None,
axis=None,
axis: Axis | None = None,
inplace: bool_t = False,
*args,
**kwargs,
) -> FrameOrSeries:
) -> FrameOrSeries | None:
"""
Trim values at input threshold(s).

Expand Down