Skip to content
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.

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

API: Disallow sets as index and columns argument in DataFrame constructor #47231

Merged
merged 5 commits into from
Aug 2, 2022

Conversation

Dr-Irv
Copy link
Contributor

@Dr-Irv Dr-Irv commented Jun 4, 2022

Note: This also allowed some typing changes removing some #type: ignore statements

@Dr-Irv Dr-Irv added Typing type annotations, mypy/pyright type checking Constructors Series/DataFrame/Index/pd.array Constructors labels Jun 4, 2022
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

Changes look good, but I think there is still not consensus on if this should go through deprecation first: #47215 (comment)

@@ -115,7 +114,7 @@
Ordered = Optional[bool]
JSONSerializable = Optional[Union[PythonScalar, List, Dict]]
Frequency = Union[str, "DateOffset"]
Axes = Collection[Any]
Axes = Union[AnyArrayLike, List, Dict, range, Sequence[str]]
Copy link
Member

Choose a reason for hiding this comment

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

Why specifically Sequence[str] and not Sequence[Hashable]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed Dict and Sequence[str] in the next commit. Sequence[Hashable] will match a string, and that is not a valid value for index or columns

pandas/core/frame.py Show resolved Hide resolved
@simonjayhawkins simonjayhawkins removed their request for review June 6, 2022 07:53
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2022

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Jul 8, 2022
@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jul 10, 2022

@jreback have merged with latest master. You were last to review - suggesting using ensure_index(), but that created test failures.

@jreback jreback added this to the 1.5 milestone Jul 10, 2022
@jreback
Copy link
Contributor

jreback commented Jul 10, 2022

hmm https://github.com/pandas-dev/pandas/runs/7271930917?check_suite_focus=true

i thinksomething changed on master for types......

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jul 10, 2022

hmm https://github.com/pandas-dev/pandas/runs/7271930917?check_suite_focus=true

i thinksomething changed on master for types......

Yes, there was a change made that made the _set_axis() parameter have too wide a type. It included Sequence, and I changed it to list

We have to avoid using Sequence in general, and Sequence[str] specifically because a plain string will match either.

Next commit addresses that.

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

I would be +0 for a deprecation, but I think this is also okay that this is mentioned in the Other API changes section. Off to @jreback

@jreback jreback merged commit cde13db into pandas-dev:main Aug 2, 2022
@jreback
Copy link
Contributor

jreback commented Aug 2, 2022

thanks @Dr-Irv lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Constructors Series/DataFrame/Index/pd.array Constructors Stale Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: Should DataFrame constructor allow sets for index and columns?
4 participants