8000 TYP: Add annotation for df.pivot by charlesdong1991 · Pull Request #32197 · pandas-dev/pandas · GitHub
[go: up one dir, main page]
Skip to content

TYP: Add annotation for df.pivot #32197

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 41 commits into from
May 18, 2020
Merged

TYP: Add annotation for df.pivot #32197

Changes from 33 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
7e461a1
remove \n from docstring
charlesdong1991 Dec 3, 2018
1314059
fix conflicts
charlesdong1991 Jan 19, 2019
8bcb313
Merge remote-tracking branch 'upstream/master'
charlesdong1991 Jul 30, 2019
24c3ede
Merge remote-tracking branch 'upstream/master'
charlesdong1991 Jan 14, 2020
dea38f2
fix issue 17038
charlesdong1991 Jan 14, 2020
cd9e7ac
revert change
charlesdong1991 Jan 14, 2020
e5e912b
revert change
charlesdong1991 Jan 14, 2020
f337468
Merge remote-tracking branch 'upstream/master' into add_signature_30928
charlesdong1991 Feb 23, 2020
1cc1225
Add signature for pivot
charlesdong1991 Feb 23, 2020
046cdc9
sorting
charlesdong1991 Feb 23, 2020
2807b63
try fix annotation
charlesdong1991 Feb 23, 2020
2fdd875
fixup
charlesdong1991 Feb 24, 2020
5453237
fixup
charlesdong1991 Feb 24, 2020
18e85bb
fixup
charlesdong1991 Feb 24, 2020
f61dcac
fixup
charlesdong1991 Feb 24, 2020
5e3ac3f
fixup
charlesdong1991 Feb 24, 2020
ffda679
fixup
charlesdong1991 Feb 24, 2020
6b81abb
fixup
charlesdong1991 Feb 24, 2020
c01f221
fixup
charlesdong1991 Feb 24, 2020
1552f4b
fixup
charlesdong1991 Feb 24, 2020
29b0605
fixup
charlesdong1991 Feb 25, 2020
4d2d6d3
fixup
charlesdong1991 Feb 25, 2020
f7fb25a
fixup
charlesdong1991 Feb 25, 2020
80e9710
Merge remote-tracking branch 'upstream/master' into add_signature_30928
charlesdong1991 Feb 25, 2020
7618b3b
fix conflict
charlesdong1991 Apr 3, 2020
d44b3df
improve annotation
charlesdong1991 Apr 3, 2020
3c3c7a4
try out
charlesdong1991 Apr 3, 2020
cb6473d
fixup
charlesdong1991 Apr 3, 2020
6586410
Merge remote-tracking branch 'upstream/master' into add_signature_30928
charlesdong1991 Apr 8, 2020
76d319a
add type ignore
charlesdong1991 Apr 8, 2020
45637fc
add comment
charlesdong1991 Apr 8, 2020
62edda6
Merge remote-tracking branch 'upstream/master' into add_signature_30928
charlesdong1991 May 17, 2020
f063a24
use cast
charlesdong1991 May 17, 2020
1ac9e0f
use convert_list_like
charlesdong1991 May 18, 2020
c17dd80
commit uncommited change
charlesdong1991 May 18, 2020
16798b1
fixup
charlesdong1991 May 18, 2020
63643aa
get rid of cast
charlesdong1991 May 18, 2020
7b10761
simplify
charlesdong1991 May 18, 2020
9870d0f
fix failed test
charlesdong1991 May 18, 2020
4e05ec0
less diff
charlesdong1991 May 18, 2020
65b45a0
Merge remote-tracking branch 'upstream/master' into add_signature_30928
charlesdong1991 May 18, 2020
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
46 changes: 35 additions & 11 deletions pandas/core/reshape/pivot.py
View file
Open in desktop
10000
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
from typing import TYPE_CHECKING, Callable, Dict, List, Tuple, Union
from typing import (
TYPE_CHECKING,
Callable,
Dict,
List,
Optional,
Sequence,
Tuple,
Union,
cast,
)

import numpy as np

from pandas._typing import Label
from pandas.util._decorators import Appender, Substitution

from pandas.core.dtypes.cast import maybe_downcast_to_dtype
Expand Down Expand Up @@ -424,16 +435,27 @@ def _convert_by(by):

@Substitution("\ndata : DataFrame")
@Appender(_shared_docs["pivot"], indents=1)
def pivot(data: "DataFrame", index=None, columns=None, values=None) -> "DataFrame":
def pivot(
data: "DataFrame",
index: Optional[Union[Label, Sequence[Label]]] = None,
columns: Optional[Union[Label, Sequence[Label]]] = None,
values: Optional[Union[Label, Sequence[Label]]] = None,
) -> "DataFrame":
if columns is None:
raise TypeError("pivot() missing 1 required argument: 'columns'")
Copy link
Member

Choose a reason for hiding this comment

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

if this is a required kwarg, should the Optional be removed from the signature? cc @simonjayhawkins not sure how to handle this given that the default value is None

Copy link
Member

Choose a reason for hiding this comment

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

There are a few considerations here.

Optional is normally included to represent a keyword argument that defaults to None. Since index is a keyword argument, columns must be a keyword argument (non-default argument follows default argument). but because Label already includes None (due to an issue with Hashable) the Optional isn't actually required in this case (This is the reason to exclude Optional from aliases in general).

If Optional was omitted, this could add to the confusion.

Since columns should allow None, if a column is named None.

so the default and check here should probably be _lib.no_default.

_lib.no_default currently resolves to Any (defined in cython with no stubs) so is compatible with anything.

If we typed _lib.no_default which is currently object() we wouldn't be able to type parameters precisely since object() wouldn't be compatible. In the future we may wish to define using Enums but this would add to the noise of the function signature from an end user perspective.

from https://www.python.org/dev/peps/pep-0484/#support-for-singleton-types-in-unions

To allow precise typing in such situations, the user should use the Union type in conjunction with the enum.Enum class provided by the standard library, so that type errors can be caught statically:

columns = columns if is_list_like(columns) else [columns]

if is_list_like(columns):
columns = cast(Sequence[Label], columns)
columns = columns
else:
columns = [columns]

if values is None:
cols: List[str] = []
cols: List[Label] = []
if index is None:
pass
elif is_list_like(index):
index = cast(Sequence[Label], index)
cols = list(index)
Copy link
Member

Choose a reason for hiding this comment

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

One of the mypy failures is that is_list_like should narrow the scope of what goes down this branch, but mypy doesn't recognize it and thinks you could potentially be calling list(index) with a Hashable. That yields error: No overload variant of "list" matches argument type "Hashable"

@jbrockmendel or @simonjayhawkins I forget, but do we have a preferred way of handling that scoping? Would it be assert isinstance(index, abc.Sequence) here?

Copy link
Member

Choose a reason for hiding this comment

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

is this something we expect mypy to get smarter about?

im increasingly leaning towards "type: ignore" on the grounds that it a) has zero runtime overhead and b) is easy to grep for in periodic see-if-we-can-remove-this sweeps

Copy link
Member

Choose a reason for hiding this comment

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

Maybe but its not implemented. The mypy issue number is 5206 so I think OK to also do #type: ignore - mypy 5206 or something similar

Copy link
Member

Choose a reason for hiding this comment

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

I think once we have #29197 in play, the #type: ignore could be a better option. so we could start adding type: ignore and once we have #29197, go back and ensure that the ignores are correct.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be assert isinstance(index, abc.Sequence) here

I think asserts are OK when narrowing to exclude a type, e.g. assert is not None, but cast is better for narrowing to ensure a type. ( unless the assert is beneficial in a run-time context)

Copy link
Member

Choose a reason for hiding this comment

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

@WillAyd @jbrockmendel #32785 was opened for this discussion. the #type: ignore option could be discussed there also. cc @jorisvandenbossche

else:
cols = [index]
Expand All @@ -443,23 +465,25 @@ def pivot(data: "DataFrame", index=None, columns=None, values=None) -> "DataFram
indexed = data.set_index(cols, append=append)
else:
if index is None:
index = [Series(data.index, name=data.index.name)]
idx_list = [Series(data.index, name=data.index.name)]
elif is_list_like(index):
index = [data[idx] for idx in index]
index = cast(Sequence[Label], index)
idx_list = [data[idx] for idx in index]
else:
index = [data[index]]
idx_list = [data[index]]

data_columns = [data[col] for col in columns]
index.extend(data_columns)
index = MultiIndex.from_arrays(index)
idx_list.extend(data_columns)
mi_index = MultiIndex.from_arrays(idx_list)

if is_list_like(values) and not isinstance(values, tuple):
# Exclude tuple because it is seen as a single column name
values = cast(Sequence[Label], values)
indexed = data._constructor(
data[values]._values, index=index, columns=values
data[values]._values, index=mi_index, columns=values
)
else:
indexed = data._constructor_sliced(data[values]._values, index=index)
indexed = data._constructor_sliced(data[values]._values, index=mi_index)
return indexed.unstack(columns)


Expand Down
0