Skip to content

fix(DataFrame): #799 to_dict #1283

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cmp0xff
Copy link
Contributor

@cmp0xff cmp0xff commented Jul 19, 2025

@cmp0xff cmp0xff force-pushed the hotfix/yw/799-to-dict branch from 94f0d80 to d041ff2 Compare July 19, 2025 20:05
@cmp0xff cmp0xff force-pushed the hotfix/yw/799-to-dict branch from 1b5bb8c to 7da086e Compare July 19, 2025 20:24
*,
into: _T_MUTABLE_MAPPING | type[_T_MUTABLE_MAPPING],
index: Literal[True] = ...,
) -> MutableMapping[Hashable, _T_MUTABLE_MAPPING]: ...
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 do now know how to get rid of the MutableMapping 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 into used for the outer mapping, the inner mapping, or both at runtime?

I assume we would need somthing like _T_MUTABLE_MAPPING[Hashable, dict[Hashable, Any]], if TypeVar would support [] (it doesn't) and if the inner mapping is always a dict (I don't know).

I'm not sure if we can annotate both the correct sub-class and also the keys/values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is into used for the outer mapping, the inner mapping, or both at runtime?

Good point!

pd.DataFrame({("a", "b"): [[1], [2, 3]]}).to_dict("index", into=defaultdict(list))

the result is actually

defaultdict(<class 'list'>, {0: {('a', 'b'): [1]}, 1: {('a', 'b'): [2, 3]}})

Only the outer mapping is the type specified by into. The inner mapping are dicts.

Copy link
Contributor Author

@cmp0xff cmp0xff Jul 20, 2025

Choose a reason for hiding this comment

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

With afb5729, now if into is defaultdict or OrderedDict, the outer mapping is of the corresponding type, and the inner mapping is correctly dict.

If into is a more generic MutableMapping, however, I do not see a way to make the outer mapping covariant and fix the inner mapping to dict at the same time. I choose to make the outer mapping just the generic MutableMapping, and the inner mapping dict.

@cmp0xff cmp0xff marked this pull request as ready for review July 19, 2025 20:33
@cmp0xff cmp0xff marked this pull request as draft July 19, 2025 23:02
@cmp0xff cmp0xff force-pushed the hotfix/yw/799-to-dict branch 3 times, most recently from 8bf75d2 to 4f1fb49 Compare July 20, 2025 13:24
@cmp0xff cmp0xff force-pushed the hotfix/yw/799-to-dict branch from 4f1fb49 to afb5729 Compare July 20, 2025 15:18
Comment on lines 425 to +456
@overload
def to_dict(
self,
orient: Literal["dict", "list", "series", "index"],
orient: Literal["index"],
*,
into: MutableMapping | type[MutableMapping],
into: defaultdict,
index: Literal[True] = ...,
) -> MutableMapping[Hashable, Any]: ...
) -> defaultdict[Hashable, dict[Hashable, Any]]: ...
@overload
def to_dict(
self,
orient: Literal["split", "tight"],
orient: Literal["index"],
*,
into: MutableMapping | type[MutableMapping],
index: bool = ...,
) -> MutableMapping[Hashable, Any]: ...
into: OrderedDict | type[OrderedDict],
index: Literal[True] = ...,
) -> OrderedDict[Hashable, dict[Hashable, Any]]: ...
@overload
def to_dict(
self,
orient: Literal["dict", "list", "series", "index"] = ...,
orient: Literal["index"],
*,
into: MutableMapping | type[MutableMapping],
index: Literal[True] = ...,
) -> MutableMapping[Hashable, Any]: ...
) -> MutableMapping[Hashable, dict[Hashable, Any]]: ...
@overload
def to_dict(
self,
orient: Literal["index"],
*,
into: type[dict] = ...,
index: Literal[True] = ...,
) -> dict[Hashable, dict[Hashable, Any]]: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Hard-coded defaultdict, OrderedDict and dict
  • Otherwise, to_dict now gives MutableMapping

Comment on lines +402 to +408
def to_dict(
self,
orient: str = ...,
*,
into: type[defaultdict],
index: Literal[True] = ...,
) -> Never: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +409 to +416
@overload
def to_dict(
self,
orient: Literal["records"],
*,
into: MutableMapping | type[MutableMapping],
into: _T_MUTABLE_MAPPING | type[_T_MUTABLE_MAPPING],
index: Literal[True] = ...,
) -> list[MutableMapping[Hashable, Any]]: ...
) -> list[_T_MUTABLE_MAPPING]: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made covariant

Comment on lines 457 to +464
@overload
def to_dict(
self,
orient: Literal["split", "tight"] = ...,
orient: Literal["dict", "list", "series"] = ...,
*,
into: MutableMapping | type[MutableMapping],
index: bool = ...,
) -> MutableMapping[Hashable, Any]: ...
into: _T_MUTABLE_MAPPING | type[_T_MUTABLE_MAPPING],
index: Literal[True] = ...,
) -> _T_MUTABLE_MAPPING: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Covariant

Comment on lines 473 to +488
@overload
def to_dict(
self,
orient: Literal["split", "tight"] = ...,
orient: Literal["split", "tight"],
*,
into: MutableMapping | type[MutableMapping],
index: bool = ...,
) -> MutableMapping[str, list]: ...
@overload
def to_dict(
self,
orient: Literal["split", "tight"],
*,
into: type[dict] = ...,
index: bool = ...,
) -> dict[Hashable, Any]: ...
) -> dict[str, list]: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old typing was wrong, now fixed.

@cmp0xff cmp0xff marked this pull request as ready for review July 20, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataFrame.to_dict(orient="index") should return a nested dictionary
2 participants