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

DriesSchaumont · 2021-04-09T20:16:04Z

closes TYP return type of DataFrame.clip not correct #40829
Ensure all linting tests pass, see here for how to run them

MarcoGorelli

Hi @DriesSchaumont

This is off to a good start!

You have overloads for:

✅ inplace: Literal[False] = ...
✅ inplace: bool = ...
✅ inplace: Literal[True]:
- when lower, upper, and axis are passed
- when lower and upper, but not axis, are passed

You're missing:

inplace: Literal[True]:
- when lower and axis, but not upper, are passed
- when upper and axis, but not lower, are passed
- when lower, but not axis or upper, is passed
- when upper, but not axis or lower, is passed
- when axis, but not lower or upper, is passed
- when none of axis, lower, or upper are passed

DriesSchaumont · 2021-04-10T12:52:48Z

Hi @DriesSchaumont

This is off to a good start!

You have overloads for:

white_check_mark inplace: Literal[False] = ...

white_check_mark inplace: bool = ...

white_check_mark inplace: Literal[True]:

when lower, upper, and axis are passed

when lower and upper, but not axis, are passed

You're missing:

inplace: Literal[True]:

when lower and axis, but not upper, are passed

when upper and axis, but not lower, are passed

when lower, but not axis or upper, is passed

when upper, but not axis or lower, is passed

when axis, but not lower or upper, is passed

when none of axis, lower, or upper are passed

Thank you for this nice overview! I will try to add these cases. I am a bit confused on how to handle the syntax for: "when upper, but not axis or lower, is passed." Is this:

def clip(self: FrameOrSeries, lower=None, upper=..., *, inplace: Literal[True], **kwargs) -> None: correct? How do I indicate that lower is not specified?

MarcoGorelli · 2021-04-10T13:19:29Z

can you try

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

?

DriesSchaumont · 2021-04-10T13:50:33Z

can you try

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

?

Thanks! I think I got them all now. Could you check if I did everything right? I am quite new to type overloading.

MarcoGorelli · 2021-04-10T14:02:57Z

Thanks! I think I got them all now. Could you check if I did everything right? I am quite new to type overloading.

Congrats for having braved this not-so-simple issue then!

To check if you've done it correctly, could you make a new file t.py (which you won't stage/commit, it'll just be there locally to check) in which you do something like what I did in #40737 (comment) ? You'll need to replace reset_index with clip, and you'll need to try with the various options of arguments which precede inplace being preset or absent, and then you should check that running mypy t.py produces the types which you were expecting

If you could do that, and paste here the content of t.py as the output from running mypy t.py, then if it all looks right then this'll be good to go

MarcoGorelli · 2021-04-10T14:23:15Z

I think the last one are correct - if inplace is just bool, then we don't know if the return type is DataFrame or None, so their union seems correct

If you repeat the first block but put inplace=False, then I'd expect you'll get all DataFrame only

DriesSchaumont · 2021-04-10T14:25:51Z

Yes, just as I left my last comment (which I then deleted), I realized my mistake. Here is the file and the output!

import pandas as pd

inplace: bool

reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, upper=4, axis=1, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=4, axis=1, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).clip(axis=1, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).clip(inplace=True))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, axis=1, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=0, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=4, axis=1, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, upper=1, inplace=True))


reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, upper=4, axis=1, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=4, axis=1, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).clip(axis=1, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).clip(inplace=False))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, axis=1, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=0, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=4, axis=1, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, upper=1, inplace=False))

reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, upper=4, axis=1, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=4, axis=1, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).clip(axis=1, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).clip(inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, axis=1, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=0, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).clip(upper=4, axis=1, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).clip(lower=0, upper=1, inplace=inplace))

t.py:6: note: Revealed type is 'None'
t.py:7: note: Revealed type is 'None'
t.py:8: note: Revealed type is 'None'
t.py:9: note: Revealed type is 'None'
t.py:10: note: Revealed type is 'None'
t.py:11: note: Revealed type is 'None'
t.py:12: note: Revealed type is 'None'
t.py:13: note: Revealed type is 'None'
t.py:14: note: Revealed type is 'None'
t.py:17: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:18: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:19: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:20: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:21: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:22: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:23: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:24: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:25: note: Revealed type is 'pandas.core.frame.DataFrame*'
t.py:27: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'
t.py:28: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'
t.py:29: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'
t.py:30: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'
t.py:31: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'
t.py:32: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'
t.py:33: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'
t.py:34: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'
t.py:35: note: Revealed type is 'Union[pandas.core.frame.DataFrame*, None]'

DriesSchaumont · 2021-04-10T14:30:52Z

@MarcoGorelli thank you for the guidance and the explanations!

MarcoGorelli

Nice one, thanks @DriesSchaumont ! Just left a small comment about how you've typed axis in some overloads but not in others, other than that this looks good to me (cc @simonjayhawkins in case you have further comments)

pandas/core/generic.py

simonjayhawkins

lgtm

simonjayhawkins · 2021-04-11T09:57:03Z

pandas/core/generic.py

+        self: FrameOrSeries,
+        lower=...,
+        upper=...,
+        axis: Axis | None = ...,


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?

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?

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.

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.

@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

* Improve clip() typing. * Update typing. * More clip overloads. * Add missing axis types

Improve clip() typing.

5f613b1

DriesSchaumont marked this pull request as draft April 9, 2021 20:58

Update typing.

fb851fd

DriesSchaumont marked this pull request as ready for review April 9, 2021 22:22

MarcoGorelli self-requested a review April 10, 2021 09:04

MarcoGorelli suggested changes Apr 10, 2021

View reviewed changes

More clip overloads.

ace7fea

MarcoGorelli mentioned this pull request Apr 10, 2021

Function overloads MarcoGorelli/pyladieslondon-sprints#29

Closed

6 tasks

MarcoGorelli added the Typing type annotations, mypy/pyright type checking label Apr 10, 2021

MarcoGorelli added this to the 1.3 milestone Apr 10, 2021

MarcoGorelli approved these changes Apr 10, 2021

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

MarcoGorelli reviewed Apr 10, 2021

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

MarcoGorelli reviewed Apr 10, 2021

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

Add missing axis types

c89e308

MarcoGorelli approved these changes Apr 10, 2021

View reviewed changes

MarcoGorelli mentioned this pull request Apr 10, 2021

TYP: Overload series/drop_dupicates #40867

Merged

4 tasks

simonjayhawkins approved these changes Apr 11, 2021

View reviewed changes

MarcoGorelli merged commit 9ab55b4 into pandas-dev:master Apr 11, 2021

DriesSchaumont deleted the fix-40829 branch April 11, 2021 11:44

MarcoGorelli mentioned this pull request Apr 12, 2021

TYP overload fillna #40737 #40887

Merged

3 tasks

JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021

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

e2e5ee8

* Improve clip() typing. * Update typing. * More clip overloads. * Add missing axis types

Uh oh!

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

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

Uh oh!

Conversation

DriesSchaumont commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoGorelli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DriesSchaumont commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoGorelli commented Apr 10, 2021

Uh oh!

DriesSchaumont commented Apr 10, 2021

Uh oh!

MarcoGorelli commented Apr 10, 2021

Uh oh!

MarcoGorelli commented Apr 10, 2021

Uh oh!

DriesSchaumont commented Apr 10, 2021

Uh oh!

DriesSchaumont commented Apr 10, 2021

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonjayhawkins left a comment

Choose a reason for hiding this comment

Uh oh!

simonjayhawkins Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

simonjayhawkins Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

simonjayhawkins Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DriesSchaumont commented Apr 9, 2021 •

edited

Loading

MarcoGorelli left a comment •

edited

Loading

DriesSchaumont commented Apr 10, 2021 •

edited

Loading