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 31 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
35 changes: 21 additions & 14 deletions pandas/core/reshape/pivot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
from typing import TYPE_CHECKING, Callable, Dict, List, Tuple, Union
from typing import TYPE_CHECKING, Callable, Dict, List, Optional, Sequence, Tuple, Union

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,42 +425,48 @@ 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[Optional[Label]]]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

does Optional[Label] mean something distinct from Label?

Copy link
Member Author

Choose a reason for hiding this comment

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

what i want to express here is that it could be an empty [], so i put an Optional here, maybe it's already covered in Label? @jbrockmendel

Copy link
Member

Choose a reason for hiding this comment

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

yes, Label is defined as Optional[Hashable]

Copy link
Contributor

Choose a reason for hiding this comment

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

i t hink would be ok to add these to _typing, maybe Labels? so this becomes Optional[Labels]

Copy link
Member

@simonjayhawkins simonjayhawkins Apr 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yeah there's a difference between Optional used for keyword args and Optional added to Label to cover None since Hashable doesn't include None.

In general, an alias in typing would not include Optional.

so Optional[Labels] would be correct and Labels would be defined accordingly. I think Union[Label, Sequence[Label]] would be sufficient.

empty is different again.

from typing import List

reveal_type([])  # Revealed type is 'builtins.list[<nothing>]'

reveal_type([1, 2])  # Revealed type is 'builtins.list[builtins.int*]'

reveal_type([1, None])  # Revealed type is 'builtins.list[Union[builtins.int, None]]'

a: List[int] = []  # no errors

where Union[builtins.int, None] == Optional[int]

so Optional is different from empty. Optional[<type>] is syntatic sugar for Union[<type>, None]. An empty list is compatible with lists of any type.

In mypy, I don't think it is possible to define a type as an empty list (it is possible for a tuple)

from https://www.python.org/dev/peps/pep-0484/#the-typing-module

The empty tuple can be typed as Tuple[()]

columns: Optional[Union[Label, Sequence[Optional[Label]]]] = None,
values: Optional[Union[Label, Sequence[Optional[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 values is None:
cols: List[str] = []
cols: List[Optional[Sequence]] = []
if index is None:
pass
elif is_list_like(index):
cols = list(index)
# Remove type ignore once mypy-5206 is implemented, same for below
Copy link
Member

Choose a reason for hiding this comment

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

can you add a "TODO: " here? makes it easier to track these down

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add an assert here to make mypy happy?

cols = list(index) # type: ignore
else:
cols = [index]
cols.extend(columns)
cols = [index] # type: ignore
cols.extend(columns) # type: ignore

append = index is None
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]
idx_list = [data[idx] for idx in index] # type: ignore
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)
data_columns = [data[col] for col in columns] # type: ignore
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
indexed = data._constructor(
data[values]._values, index=index, columns=values
data[values]._values, index=mi_index, columns=values # type: ignore
)
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