TYP overload fillna #40737 #40887

LarWong · 2021-04-11T23:15:42Z

closes TYP overload fillna #40737
tests added / passed
Ensure all linting tests pass, see here for how to run them

…v#40737

MarcoGorelli

You don't need all these overloads - you just need:

one for Literal[False]=...
one for bool=...
one for each combination of default arguments which precede inplace

that should work out to 10 in total

LarWong · 2021-04-12T20:23:40Z

@MarcoGorelli Oh ok, I was under the impression that I needed an overload for each combination. I'll change it.

MarcoGorelli · 2021-04-12T21:02:01Z

Nice! And great that this makes a couple of casts redundant

Could include the output from running something like what was done in #40860 (comment) ? I think you might need to always include =... for default arguments which come after inplace for it to work, but we'll see

LarWong · 2021-04-12T21:04:22Z

Sure, I'll post it soon

LarWong · 2021-04-12T21:14:30Z

@MarcoGorelli Yes, you were correct. =... was needed for it to work

LarWong · 2021-04-12T21:27:22Z

Here are the inputs to and the outputs of mypy:

# DataFrame_t.py
import pandas as pd

inplace : bool

reveal_type(pd.DataFrame([1,2,3]).fillna(inplace=False))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).fillna(method='pad', inplace=False))
reveal_type(pd.DataFrame([1,2,3]).fillna(axis=0, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, method='pad', inplace=False))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, axis=0, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).fillna(method='pad', axis=0, inplace=False))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, method='pad', axis=0, inplace=False))

reveal_type(pd.DataFrame([1,2,3]).fillna(inplace=True))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).fillna(method='pad', inplace=True))
reveal_type(pd.DataFrame([1,2,3]).fillna(axis=0, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, method='pad', inplace=True))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, axis=0, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).fillna(method='pad', axis=0, inplace=True))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, method='pad', axis=0, inplace=True))

reveal_type(pd.DataFrame([1,2,3]).fillna(inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).fillna(method='pad', inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).fillna(axis=0, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, method='pad', inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, axis=0, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).fillna(method='pad', axis=0, inplace=inplace))
reveal_type(pd.DataFrame([1,2,3]).fillna(value=0, method='pad', axis=0, inplace=inplace))

Output:

DataFrame_t.py:5: note: Revealed type is 'pandas.core.frame.DataFrame'
DataFrame_t.py:6: note: Revealed type is 'pandas.core.frame.DataFrame'
DataFrame_t.py:7: note: Revealed type is 'pandas.core.frame.DataFrame'
DataFrame_t.py:8: note: Revealed type is 'pandas.core.frame.DataFrame'
DataFrame_t.py:9: note: Revealed type is 'pandas.core.frame.DataFrame'
DataFrame_t.py:10: note: Revealed type is 'pandas.core.frame.DataFrame'
DataFrame_t.py:11: note: Revealed type is 'pandas.core.frame.DataFrame'
DataFrame_t.py:12: note: Revealed type is 'pandas.core.frame.DataFrame'
DataFrame_t.py:14: note: Revealed type is 'None'
DataFrame_t.py:15: note: Revealed type is 'None'
DataFrame_t.py:16: note: Revealed type is 'None'
DataFrame_t.py:17: note: Revealed type is 'None'
DataFrame_t.py:18: note: Revealed type is 'None'
DataFrame_t.py:19: note: Revealed type is 'None'
DataFrame_t.py:20: note: Revealed type is 'None'
DataFrame_t.py:21: note: Revealed type is 'None'
DataFrame_t.py:23: note: Revealed type is 'Union[pandas.core.frame.DataFrame, None]'
DataFrame_t.py:24: note: Revealed type is 'Union[pandas.core.frame.DataFrame, None]'
DataFrame_t.py:25: note: Revealed type is 'Union[pandas.core.frame.DataFrame, None]'
DataFrame_t.py:26: note: Revealed type is 'Union[pandas.core.frame.DataFrame, None]'
DataFrame_t.py:27: note: Revealed type is 'Union[pandas.core.frame.DataFrame, None]'
DataFrame_t.py:28: note: Revealed type is 'Union[pandas.core.frame.DataFrame, None]'
DataFrame_t.py:29: note: Revealed type is 'Union[pandas.core.frame.DataFrame, None]'
DataFrame_t.py:30: note: Revealed type is 'Union[pandas.core.frame.DataFrame, None]'

# Series_t.py
import pandas as pd

inplace : bool

reveal_type(pd.Series([1,2,3]).fillna(inplace=False))
reveal_type(pd.Series([1,2,3]).fillna(value=0, inplace=False))
reveal_type(pd.Series([1,2,3]).fillna(method='pad', inplace=False))
reveal_type(pd.Series([1,2,3]).fillna(axis=0, inplace=False))
reveal_type(pd.Series([1,2,3]).fillna(value=0, method='pad', inplace=False))
reveal_type(pd.Series([1,2,3]).fillna(value=0, axis=0, inplace=False))
reveal_type(pd.Series([1,2,3]).fillna(method='pad', axis=0, inplace=False))
reveal_type(pd.Series([1,2,3]).fillna(value=0, method='pad', axis=0, inplace=False))

reveal_type(pd.Series([1,2,3]).fillna(inplace=True))
reveal_type(pd.Series([1,2,3]).fillna(value=0, inplace=True))
reveal_type(pd.Series([1,2,3]).fillna(method='pad', inplace=True))
reveal_type(pd.Series([1,2,3]).fillna(axis=0, inplace=True))
reveal_type(pd.Series([1,2,3]).fillna(value=0, method='pad', inplace=True))
reveal_type(pd.Series([1,2,3]).fillna(value=0, axis=0, inplace=True))
reveal_type(pd.Series([1,2,3]).fillna(method='pad', axis=0, inplace=True))
reveal_type(pd.Series([1,2,3]).fillna(value=0, method='pad', axis=0, inplace=True))

reveal_type(pd.Series([1,2,3]).fillna(inplace=inplace))
reveal_type(pd.Series([1,2,3]).fillna(value=0, inplace=inplace))
reveal_type(pd.Series([1,2,3]).fillna(method='pad', inplace=inplace))
reveal_type(pd.Series([1,2,3]).fillna(axis=0, inplace=inplace))
reveal_type(pd.Series([1,2,3]).fillna(value=0, method='pad', inplace=inplace))
reveal_type(pd.Series([1,2,3]).fillna(value=0, axis=0, inplace=inplace))
reveal_type(pd.Series([1,2,3]).fillna(method='pad', axis=0, inplace=inplace))
reveal_type(pd.Series([1,2,3]).fillna(value=0, method='pad', axis=0, inplace=inplace))

Output:

Series_t.py:5: note: Revealed type is 'pandas.core.series.Series'
Series_t.py:6: note: Revealed type is 'pandas.core.series.Series'
Series_t.py:7: note: Revealed type is 'pandas.core.series.Series'
Series_t.py:8: note: Revealed type is 'pandas.core.series.Series'
Series_t.py:9: note: Revealed type is 'pandas.core.series.Series'
Series_t.py:10: note: Revealed type is 'pandas.core.series.Series'
Series_t.py:11: note: Revealed type is 'pandas.core.series.Series'
Series_t.py:12: note: Revealed type is 'pandas.core.series.Series'
Series_t.py:14: note: Revealed type is 'None'
Series_t.py:15: note: Revealed type is 'None'
Series_t.py:16: note: Revealed type is 'None'
Series_t.py:17: note: Revealed type is 'None'
Series_t.py:18: note: Revealed type is 'None'
Series_t.py:19: note: Revealed type is 'None'
Series_t.py:20: note: Revealed type is 'None'
Series_t.py:21: note: Revealed type is 'None'
Series_t.py:23: note: Revealed type is 'Union[pandas.core.series.Series, None]'
Series_t.py:24: note: Revealed type is 'Union[pandas.core.series.Series, None]'
Series_t.py:25: note: Revealed type is 'Union[pandas.core.series.Series, None]'
Series_t.py:26: note: Revealed type is 'Union[pandas.core.series.Series, None]'
Series_t.py:27: note: Revealed type is 'Union[pandas.core.series.Series, None]'
Series_t.py:28: note: Revealed type is 'Union[pandas.core.series.Series, None]'
Series_t.py:29: note: Revealed type is 'Union[pandas.core.series.Series, None]'
Series_t.py:30: note: Revealed type is 'Union[pandas.core.series.Series, None]'

MarcoGorelli

Thanks @LarWong ! Looks good, I just have a question about:

value: Scalar | dict | Series | DataFrame | None

Where did you get this from? Was it from inspection? cc @simonjayhawkins is this correct?

MarcoGorelli · 2021-04-13T09:41:36Z

pandas/core/frame.py

+    @overload
+    def fillna(
+        self,
+        value: Scalar | dict | Series | DataFrame | None = ...,


you've typed value in the overloads, perhaps let's type it in the function signature too? (line 5128)

I can modify it, but let's wait until someone has confirmed the typing of value first.

@LarWong perhaps let's revert typing value here, that can be done separately (and we'll probably need to be more precise than dict), the rest looks good

@MarcoGorelli Sorry, but I'm not sure what you mean by revert typing. Do you mean:

@overload def fillna( self, value,

As in not typing value in the overloads?

@MarcoGorelli Ok done! The outputs from mypy are the same after modifications (see above).

jreback · 2021-04-13T11:58:00Z

pandas/core/frame.py

+    @overload
+    def fillna(
+        self,
+        value: Scalar | dict | Series | DataFrame | None = ...,


can we make an alias for this

jreback · 2021-04-13T11:58:34Z

pandas/core/frame.py

@@ -5007,6 +5007,121 @@ def rename(
            errors=errors,
        )

+    @overload


can we not put these in .pyi?

I was told to add them directly to these files since existing overloads were already there

@jreback yes, that would be great! Thing is though, the .pyi files require you to define all methods of a module

Given the sheer number of methods this module has, I'd suggest taking this PR with the overloads here, and then moving the overloads (along with annotations for all other methods) to a pandas/core/frame.pyi file

@LarWong I'll get back to you on typing value

can we not put these in .pyi?

I don't think we want to go the route of using stubs for python files

@jreback yes, that would be great! Thing is though, the .pyi files require you to define all methods of a module

not that I think it should be done here, but it is possible to partially type a module using stubs.

https://github.com/python/typeshed/blob/master/CONTRIBUTING.md#incomplete-stubs

Partial modules (i.e. modules that are missing some or all classes, functions, or attributes) must include a top-level getattr() function marked with an # incomplete comment (see example below).

Thanks @simonjayhawkins , didn't know that was possible. Why not use partial stubs for overloads? Because some methods, like Series.drop

pandas/pandas/core/series.py

Lines 4478 to 4487 in 84d9c5e

def drop(

self,

labels=None,

axis=0,

index=None,

columns=None,

level=None,

inplace=False,

errors="raise",

) -> Series:

have 5 (five!) arguments with defaults before inplace, leading to...34 overloads 🤯 ! And even if we disallowed labels being passed as None in that one, that would still leave us with 18 overloads!

For typing value here, do you think Scalar | Mapping[Hashable, Scalar] | Series | DataFrame | None would be correct?

The problem is that if a stub file is present it takes precedence over the python file. so we cannot ensure internal consistency and need to duplicate the type annotations to be able to check the functions in the module itself.

have 5 (five!) arguments with defaults before inplace, leading to...34 overloads

I think we are planning to to drop the inplace argument, hopefully pandas 2.0 and we won't need all these overloads. #16529

LarWong · 2021-04-13T12:50:57Z

Thanks @LarWong ! Looks good, I just have a question about:
value: Scalar | dict | Series | DataFrame | None
Where did you get this from? Was it from inspection? cc @simonjayhawkins is this correct?

@MarcoGorelli For value, those were the types suggested by the documentation.

MarcoGorelli

Thanks @LarWong !

Will try to either move overloads to .pyi files and have some way of keeping them in sync, or turning off the black formatter and putting some noqas in, tomorrow

* TYP: Added overloads for fillna() in frame.py and series.py * TYP: Added overloads for fillna() in frame.py and series.py pandas-dev#40737 * TYP: Added fillna() overloads to generic.py pandas-dev#40727 * TYP: removed generic overloads pandas-dev#40737 * fixed redundant cast error * reverting prior changes * remove cast again * removed unnecessary overloads in frame.py and series.py * fixed overloads * reverted value typing * remove extra types (lets keep this to overloads) Co-authored-by: Marco Gorelli <marcogorelli@protonmail.com>

LarWong added 5 commits April 11, 2021 16:33

TYP: Added overloads for fillna() in frame.py and series.py

79f6b50

TYP: Added overloads for fillna() in frame.py and series.py pandas-de…

9b6966a

…v#40737

TYP: Added fillna() overloads to generic.py pandas-dev#40727

fce2d11

Merge branch 'typ-fillna' of github.com:LarWong/pandas into typ-fillna

bd93f31

TYP: removed generic overloads pandas-dev#40737

16b42e7

LarWong marked this pull request as draft April 11, 2021 23:57

LarWong added 4 commits April 12, 2021 10:50

fixed redundant cast error

2ab41f8

Merge remote-tracking branch 'upstream/master' into typ-fillna

d6cc081

reverting prior changes

a641df2

remove cast again

45cc60c

LarWong marked this pull request as ready for review April 12, 2021 19:38

MarcoGorelli suggested changes Apr 12, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into typ-fillna

1c33bd5

LarWong marked this pull request as draft April 12, 2021 20:32

removed unnecessary overloads in frame.py and series.py

b6a4fac

fixed overloads

2adcaf7

LarWong marked this pull request as ready for review April 12, 2021 22:59

MarcoGorelli self-requested a review April 13, 2021 09:33

MarcoGorelli reviewed Apr 13, 2021

View reviewed changes

jreback added the Typing type annotations, mypy/pyright type checking label Apr 13, 2021

jreback requested changes Apr 13, 2021

View reviewed changes

jreback added the Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate label Apr 13, 2021

LarWong added 2 commits April 15, 2021 10:50

Merge remote-tracking branch 'upstream/master' into typ-fillna

4b7744e

reverted value typing

0c3d28d

MarcoGorelli self-requested a review April 15, 2021 17:06

remove extra types (lets keep this to overloads)

1c8dd1a

MarcoGorelli approved these changes Apr 15, 2021

View reviewed changes

MarcoGorelli added this to the 1.3 milestone Apr 15, 2021

MarcoGorelli merged commit 8fd8c8b into pandas-dev:master Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TYP overload fillna #40737 #40887

TYP overload fillna #40737 #40887

LarWong commented Apr 11, 2021 •

edited

Loading

MarcoGorelli left a comment

LarWong commented Apr 12, 2021

MarcoGorelli commented Apr 12, 2021

LarWong commented Apr 12, 2021

LarWong commented Apr 12, 2021

LarWong commented Apr 12, 2021

MarcoGorelli left a comment •

edited

Loading

MarcoGorelli Apr 13, 2021

LarWong Apr 13, 2021

MarcoGorelli Apr 14, 2021

LarWong Apr 14, 2021 •

edited

Loading

MarcoGorelli Apr 15, 2021

LarWong Apr 15, 2021

jreback Apr 13, 2021

jreback Apr 13, 2021

LarWong Apr 13, 2021

MarcoGorelli Apr 13, 2021

MarcoGorelli Apr 13, 2021

simonjayhawkins Apr 14, 2021

MarcoGorelli Apr 14, 2021

simonjayhawkins Apr 14, 2021 •

edited

Loading

simonjayhawkins Apr 14, 2021

LarWong commented Apr 13, 2021 •

edited

Loading

MarcoGorelli left a comment

	def drop(
	self,
	labels=None,
	axis=0,
	index=None,
	columns=None,
	level=None,
	inplace=False,
	errors="raise",
	) -> Series:

TYP overload fillna #40737 #40887

TYP overload fillna #40737 #40887

Conversation

LarWong commented Apr 11, 2021 • edited Loading

MarcoGorelli left a comment

Choose a reason for hiding this comment

LarWong commented Apr 12, 2021

MarcoGorelli commented Apr 12, 2021

LarWong commented Apr 12, 2021

LarWong commented Apr 12, 2021

LarWong commented Apr 12, 2021

MarcoGorelli left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LarWong Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LarWong commented Apr 13, 2021 • edited Loading

MarcoGorelli left a comment

Choose a reason for hiding this comment

LarWong commented Apr 11, 2021 •

edited

Loading

MarcoGorelli left a comment •

edited

Loading

LarWong Apr 14, 2021 •

edited

Loading

simonjayhawkins Apr 14, 2021 •

edited

Loading

LarWong commented Apr 13, 2021 •

edited

Loading