Skip to content

TYP: fix incorrect mypy error in reshape.py #45127

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 3 commits into from
Dec 31, 2021
Merged

TYP: fix incorrect mypy error in reshape.py #45127

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a63b165
TYP: fix incorrect mypy error in reshape.py
Dec 30, 2021
58b4f63
TYP: define new_leveln as FrozenList or List
Dec 30, 2021
1fc040f
Merge remote-tracking branch 'upstream/master' into fix_mypy_err_resh…
Dec 30, 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
8 changes: 5 additions & 3 deletions pandas/core/reshape/reshape.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
Index,
MultiIndex,
)
from pandas.core.indexes.frozen import FrozenList
from pandas.core.series import Series
from pandas.core.sorting import (
compress_group_index,
Expand Down Expand Up @@ -316,15 +317,16 @@ def get_new_columns(self, value_columns: Index | None):
stride = len(self.removed_level) + self.lift
width = len(value_columns)
propagator = np.repeat(np.arange(width), stride)

new_levels: FrozenList | list[Index]

if isinstance(value_columns, MultiIndex):
new_levels = value_columns.levels + (self.removed_level_full,)
new_names = value_columns.names + (self.removed_name,)

new_codes = [lab.take(propagator) for lab in value_columns.codes]
else:
# error: Incompatible types in assignment (expression has type "List[Any]",
# variable has type "FrozenList")
new_levels = [ # type: ignore[assignment]
new_levels = [
Copy link
Member

Choose a reason for hiding this comment

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

value_columns.levels has type FrozenList. In general: Mypy complains if we have unused ignore comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah missed seeing that during my review. Ill close this PR, thanks

Copy link
Member

Choose a reason for hiding this comment

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

@kashifkhan you might be able to get rid of the # type: ignore comment by annotating new_levels where it's defined (line 320)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarcoGorelli hmm I can do that, but then on line 327 new_levels can also be a List of Index.

My current thought is to annotate new_levels to be a FrozenList on line 320 and then have a new variable new_levels_list as a List[Index] on line 327 . Then change the return statement based on which one is populated ...thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

You can just use new_levels: FrozenList | list[Index] I think?

Copy link
Member

Choose a reason for hiding this comment

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

or sequence[Index] if it doesn't need to be a list?

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 a Sequence should be ok, but since we are only using the variable only within the function we should narrow the type down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phofl went with list since it is only generated within the function & then passed on to the MultiIndex constructor which needs an Iterable.

value_columns,
self.removed_level_full,
]
Expand Down