Maintain Dict Ordering with Concat #21512

SaturninoMateus · 2018-06-16T19:26:07Z

closes concat does not respect order of an OrderedDict #21510
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

although the set is instance of dict, it should not sort if it is also instance of OrderedDict

pep8speaks · 2018-06-16T19:26:09Z

Hello @SaturninoMateus! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on June 21, 2018 at 02:03 Hours UTC

codecov · 2018-06-16T21:50:49Z

Codecov Report

Merging #21512 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #21512      +/-   ##
==========================================
+ Coverage   91.92%   91.92%   +<.01%     
==========================================
  Files         153      153              
  Lines       49563    49566       +3     
==========================================
+ Hits        45559    45562       +3     
  Misses       4004     4004

Flag	Coverage Δ
#multiple	`90.32% <100%> (ø)`	⬆️
#single	`41.8% <25%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/reshape/concat.py	`97.61% <100%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 506935c...a7aa052. Read the comment docs.

WillAyd · 2018-06-16T22:30:42Z

Is this in reference to a particular issue?

SaturninoMateus · 2018-06-16T22:39:40Z

Sorry?

WillAyd · 2018-06-16T22:40:26Z

What issue is this solving? Typically all pull requests (except for simple doc updates) refer to an issue on the issue tracker

SaturninoMateus · 2018-06-16T22:42:10Z

Issue #21510

WillAyd · 2018-06-16T22:45:45Z

OK thanks. For future PRs please add that reference in your original comment (I've updated it for you this time).

As per the checklist, you are also going to need tests and a whatsnew entry for 0.23.2

topper-123 · 2018-06-16T23:18:41Z

pandas/core/reshape/concat.py

@@ -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)


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.

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

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

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...

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.

@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.

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

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?

WillAyd · 2018-06-17T01:41:27Z

pandas/tests/reshape/test_concat.py

+        exp_list = [('First', 0), ('First', 1), ('First', 2),
+                    ('Another', 0), ('Another', 1),
+                    ('Another', 2), ('Another', 3)]
+        assert ps_list == exp_list


Typically with test cases we have a pattern of using result and expected as the variable names. In this case, you should create an expected variable that is exactly the Series you are looking for and then us tm.assert_series_equal(result, expected) to ensure the proper outcome

It is ok if I just change the variables names (expected and result), or I should also create the Series object?

Create the Series

I'm trying to figure it out how I can create the Series with different sizes without using the concat by itself.

I tried to create like this:

a = pd.Series(range(3),range(3)) b = pd.Series(range(4),range(4)) expected = pd.Series([a,b],index=['First','Another'])

WillAyd · 2018-06-17T01:42:39Z

doc/source/whatsnew/v0.23.2.txt

@@ -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`)
-
+- Bug in :class:`_Concatenator` should not sort Ordered Dictionaries (:issue:`21510`)


Can you come up with some less technical? This is too focused on the technical aspect of the change, but you really should be writing something that a casual user could read and understand in the whatsnew

Agreed. I would just say that the bug was causing pd.concat to not respect order present in an ordered dictionary object (something along those lines).

Please, check my last commit.

Getting closer but you should remove any reference to _Concatenator as it is a private class. I would be fine with almost a copy/paste of what @gfyoung suggested above as well

WillAyd · 2018-06-17T23:03:29Z

pandas/tests/reshape/test_concat.py

+        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)


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

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

jreback · 2018-06-18T10:13:57Z

doc/source/whatsnew/v0.23.2.txt

@@ -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`)
-
+- Bug in :class:`_Concatenator` should maintain dict ordering when :meth:`concat` is called (:issue:`2151`)


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

jreback · 2018-06-18T10:18:58Z

pandas/core/reshape/concat.py

@@ -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)


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.

jreback · 2018-06-18T10:20:02Z

pandas/tests/reshape/test_concat.py

+        # GH 21510
+        result = pd.concat(OrderedDict([('First', pd.Series(range(3))),
+                                        ('Another', pd.Series(range(4)))]))
+        a = pd.Series(range(3), range(3))


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

Why should I specify that?

jreback · 2018-06-18T10:21:36Z

doc/source/whatsnew/v0.23.2.txt

@@ -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`)
-
+- Bug in :class:`_Concatenator` should maintain dict ordering when :meth:`concat` is called (:issue:`2151`)
+-


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

pandas-dev#21510

jreback · 2018-06-21T00:25:31Z

doc/source/whatsnew/v0.23.2.txt

@@ -93,4 +93,6 @@ Bug Fixes

 **Other**

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


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

# Conflicts: # doc/source/whatsnew/v0.23.2.txt

jreback · 2018-06-21T10:39:22Z

.gitignore

@@ -110,3 +110,4 @@ doc/source/styled.xlsx
 doc/source/templates/
 env/
 doc/source/savefig/
+*my-dev-test.py


what is this from?

jreback · 2018-06-21T10:39:47Z

pandas/core/reshape/concat.py

@@ -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)


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

jreback · 2018-10-11T01:52:22Z

closing as stale, if you want to continue working, pls ping and we can re-open. you will need to merge master.

Fixes pandas-dev#21510 Remove sort if its already an ordered dict

87bb1ac

Adding a space and new line to match PEP8. pandas-dev#21510

f9d105e

WillAyd changed the title ~~Fixes #21510 Remove sort if its already an ordered dict~~ Remove sort if its already an ordered dict Jun 16, 2018

WillAyd changed the title ~~Remove sort if its already an ordered dict~~ Maintain Dict Ordering with Concat Jun 16, 2018

topper-123 requested changes Jun 16, 2018

View reviewed changes

SaturninoMateus added 2 commits June 16, 2018 22:17

Adding test case for concat OrderedDict and updating . pandas-dev#21510

b044e04

Fixing v0.23.2.txt syntax . pandas-dev#21510

eaa7cc0

WillAyd requested changes Jun 17, 2018

View reviewed changes

gfyoung added Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Jun 17, 2018

Improving test case for orderedDict and whatsnew info. pandas-dev#21510

6869e2c

WillAyd requested changes Jun 17, 2018

View reviewed changes

jreback requested changes Jun 18, 2018

View reviewed changes

SaturninoMateus added 2 commits June 19, 2018 02:50

Refactoring test case code for orderedDict and improving whatsnew info.

e9561fe

pandas-dev#21510

Fixing spaces to match with PEP8. pandas-dev#21510

53aca81

SaturninoMateus force-pushed the my-dev branch from d29f782 to 53aca81 Compare June 19, 2018 07:29

jreback requested changes Jun 21, 2018

View reviewed changes

Merging upstream/master into my dev branch

726755f

# Conflicts: # doc/source/whatsnew/v0.23.2.txt

SaturninoMateus force-pushed the my-dev branch from 9d634ca to 726755f Compare June 21, 2018 01:34

Adding info about pd.concat method into whatsnew 0.24.0 pandas-dev#21510

a7aa052

jreback requested changes Jun 21, 2018

View reviewed changes

jreback closed this Oct 11, 2018

alexander-ponomaroff mentioned this pull request Feb 8, 2019

Fix concat not respecting order of OrderedDict #25224

Merged

3 tasks

Maintain Dict Ordering with Concat #21512

Maintain Dict Ordering with Concat #21512

Conversation

SaturninoMateus commented Jun 16, 2018 • edited Loading

pep8speaks commented Jun 16, 2018 • edited Loading

Comment last updated on June 21, 2018 at 02:03 Hours UTC

codecov bot commented Jun 16, 2018 • edited Loading

Codecov Report

WillAyd commented Jun 16, 2018

SaturninoMateus commented Jun 16, 2018

WillAyd commented Jun 16, 2018

SaturninoMateus commented Jun 16, 2018 • edited Loading

WillAyd commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaturninoMateus Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 11, 2018

SaturninoMateus commented Jun 16, 2018 •

edited

Loading

pep8speaks commented Jun 16, 2018 •

edited

Loading

codecov bot commented Jun 16, 2018 •

edited

Loading

SaturninoMateus commented Jun 16, 2018 •

edited

Loading

SaturninoMateus Jun 17, 2018 •

edited

Loading