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

Show file tree
Hide file tree
Changes from 5 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
3 changes: 2 additions & 1 deletion doc/source/whatsnew/v0.23.2.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,5 @@ Bug Fixes
**Other**

- Bug in :meth:`Series.nlargest` for signed and unsigned integer dtypes when the minimum value is present (:issue:`21426`)
Copy link
Contributor

Choose a reason for hiding this comment

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

you picked up some extra text here. also move to 0.24

-
- Bug in :class:`_Concatenator` should maintain dict ordering when :meth:`concat` is called (:issue:`2151`)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a user facing comment, meaning it should only use the public API.

Copy link
Author

Choose a reason for hiding this comment

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

Got it

-
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to go into 0.24.0 as even though its a bug fix, its a jaring one, should be in reshaping.

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
12 changes: 12 additions & 0 deletions pandas/tests/reshape/test_concat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from collections import OrderedDict
from warnings import catch_warnings
from itertools import combinations, product

Expand Down Expand Up @@ -1294,6 +1295,17 @@ def test_concat_rename_index(self):
tm.assert_frame_equal(result, exp)
assert result.index.names == exp.index.names

def test_concat_with_ordered_dict(self):
# GH 21510
result = pd.concat(OrderedDict([('First', pd.Series(range(3))),
('Another', pd.Series(range(4)))]))
a = pd.Series(range(3), range(3))
Copy link
Contributor

Choose a reason for hiding this comment

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

if you use the 2 form constructor, then specify index=. I don't think it actually helps here though.

Copy link
Author

Choose a reason for hiding this comment

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

Why should I specify that?

b = pd.Series(range(4), range(4))
a.index = pd.MultiIndex.from_tuples([('First', v) for v in a.index])
b.index = pd.MultiIndex.from_tuples([('Another', v) for v in b.index])
expected = a.append(b)
Copy link
Member

Choose a reason for hiding this comment

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

It would be easier if you just constructed the Series directly. You could doing something like index = pd.MultiIndex.from_records(...) and then expected = pd.Series(..., index=index) to make this as succinct and clear as possible

Copy link
Author

Choose a reason for hiding this comment

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

AttributeError: type object 'MultiIndex' has no attribute 'from_records'

tm.assert_series_equal(result, expected)

def test_crossed_dtypes_weird_corner(self):
columns = ['A', 'B', 'C', 'D']
df1 = DataFrame({'A': np.array([1, 2, 3, 4], dtype='f8'),
Expand Down