Skip to content

Maintain Dict Ordering with Concat #21512

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

Closed
wants to merge 9 commits into from
Closed

Maintain Dict Ordering with Concat #21512

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
87bb1ac
Fixes #21510 Remove sort if its already an ordered dict
SaturninoMateus Jun 16, 2018
f9d105e
Adding a space and new line to match PEP8. #21510
SaturninoMateus Jun 16, 2018
b044e04
Adding test case for concat OrderedDict and updating . #21510
SaturninoMateus Jun 17, 2018
eaa7cc0
Fixing v0.23.2.txt syntax . #21510
SaturninoMateus Jun 17, 2018
6869e2c
Improving test case for orderedDict and whatsnew info. #21510
SaturninoMateus Jun 17, 2018
e9561fe
Refactoring test case code for orderedDict and improving whatsnew inf…
SaturninoMateus Jun 19, 2018
53aca81
Fixing spaces to match with PEP8. #21510
SaturninoMateus Jun 19, 2018
726755f
Merging upstream/master into my dev branch
SaturninoMateus Jun 21, 2018
a7aa052
Adding info about pd.concat method into whatsnew 0.24.0 #21510
SaturninoMateus Jun 21, 2018
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
7 changes: 5 additions & 2 deletions pandas/core/reshape/concat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""
concat routines
"""

from collections import OrderedDict
import numpy as np
from pandas import compat, DataFrame, Series, Index, MultiIndex
from pandas.core.index import (_get_objs_combined_axis,
Expand Down Expand Up @@ -250,7 +250,10 @@ def __init__(self, objs, axis=0, join='outer', join_axes=None,

if isinstance(objs, dict):
if keys is None:
keys = sorted(objs)
if not isinstance(objs, OrderedDict):
keys = sorted(objs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Normal dicts are ordered too in python3.6+, that must be checked too. There is a PY36 constant somewhere, that you can use for this check.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we already took a stance on this as a project, but the orderedness of dicts in 3.6 is considered an implementation detail of cpython, while it will be a language feature starting from 3.7 . So in principle in 3.6 users should not rely on it, and we should not assume they do.

@topper-123 's comment still holds I think, but for 3.7

Copy link
Member

Choose a reason for hiding this comment

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

Well we've already relied on that implementation detail in other changes (see #19830, #19859, #19884) so I'd be +1 on relying on that here as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, given that Python 3.6 is at 3.6.5 now, and its's being formalised in 3.7, I think this is quite safe. They're not going to change implementation now...

Copy link
Contributor

Choose a reason for hiding this comment

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

This is very simliar to in structure to pandas.core.common._dict_keys_to_ordered_list. structure like this. see if we can consolidate all of these try/sorting routings.

Copy link
Author

Choose a reason for hiding this comment

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

@jreback you mean use: keys = com._dict_keys_to_ordered_list(objs) ? If so, I'm afraid we'll need to refactor _dict_keys_to_ordered_list because it assumes that PY36 has sorted dict which I think its not always true, and will break test_concat_dict test case.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that's the point i am raising, I want a refactor here.

Copy link
Author

Choose a reason for hiding this comment

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

So, from _dict_keys_to_ordered_list method, i suggest change the conditionif PY36 or isinstance(mapping, OrderedDict): to if PY37 or isinstance(mapping, OrderedDict): then, remove test_constructor_dict_order_insertion and test_constructor_dict_order. What do you think about it?

else:
keys = objs
objs = [objs[k] for k in keys]
else:
objs = list(objs)
Expand Down