Fix concat not respecting order of OrderedDict
State: closed

- [x] closes #21510
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


Labels:
name: Bug
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
Hello @alexander-ponomaroff! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:









There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-03-12 16:12:36 UTC
@jreback @toobaz I've reviewed your comments in Pull Request #21512 that went stale, and created a fix based on what I observed. As @toobaz said, the ordering of dicts in 3.6 is an implementation detail of cpython, and users should not rely on it in 3.6. Based on this, I changed dict_keys_to_ordered_list to use PY37 instead of PY36 and changed the tests that were failing to use PY37 as well.

Please let me know if this is the wrong way of going about this. If my changes are acceptable, I will change the comments in the tests and dict_keys_to_ordered_list to use 3.7 instead of 3.6.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=h1) Report
> Merging [#25224](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0eddba88332e9b7f41b838e503c155f7ebe4d9a1?src=pr&el=desc) will **decrease** coverage by `49.87%`.
> The diff coverage is `66.66%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25224/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           master   #25224       +/-   ##
===========================================
- Coverage   92.15%   42.28%   -49.88%     
===========================================
  Files         166      166               
  Lines       52294    52294               
===========================================
- Hits        48193    22111    -26082     
- Misses       4101    30183    +26082
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `42.28% <66.66%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9jb25jYXQucHk=) | `44% <0%> (-53.2%)` | :arrow_down: |
| [pandas/core/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tbW9uLnB5) | `68.81% <100%> (-29.57%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.35%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| ... and [125 more](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=footer). Last update [0eddba8...d4e70ef](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=h1) Report
> Merging [#25224](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/26d991fc4e568432c3264151265c0a3ea9a6f133?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25224/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #25224   +/-   ##
=======================================
  Coverage   91.28%   91.28%           
=======================================
  Files         173      173           
  Lines       52965    52965           
=======================================
  Hits        48348    48348           
  Misses       4617     4617
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.86% <100%> (ø)` | :arrow_up: |
| #single | `41.74% <0%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9nZW5lcmljLnB5) | `87% <100%> (ø)` | :arrow_up: |
| [pandas/core/reshape/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25224/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9jb25jYXQucHk=) | `97.2% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=footer). Last update [26d991f...1557c21](https://codecov.io/gh/pandas-dev/pandas/pull/25224?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> As @toobaz said, the ordering of dicts in 3.6 is an implementation detail of cpython, and users should not rely on it in 3.6

Might be out of the loop but wasn't it declared that at first but since then guaranteed as a feature? If so I don't think we need to make distinction between 3.6 and 3.7 here
@jreback Thank you for the feedback, I will make changes tomorrow.
@WillAyd I get an error in one of the tests that you created. Would you have any suggestions for me? test_groupby_agg_ohlc_non_first() inside pandas/tests/groupby/test_groupby.py.
Looks like its the ordering of the columns in the resulting data frame. I think this is actually preferable because it respects the order supplied by the user, so I think OK to update test accordingly
@WillAyd Thank you for such a fast response. Just to confirm, how do you feel about the change I made?
Right - I’m asking if there’s a way to keep the behavior here consistent in the code base rather than setting different expectations in the test

> On Feb 12, 2019, at 2:40 PM, Alexander Ponomaroff <notifications@github.com> wrote:
> 
> @alexander-ponomaroff commented on this pull request.
> 
> In pandas/tests/groupby/test_groupby.py <https://github.com/pandas-dev/pandas/pull/25224#discussion_r256177741>:
> 
> > @@ -1695,6 +1695,10 @@ def test_groupby_agg_ohlc_non_first():
>          ('foo', 'sum', 'foo'))), index=pd.date_range(
>              '2018-01-01', periods=2, freq='D'))
>  
> +    if compat.PY36:
> @WillAyd <https://github.com/WillAyd> What do you mean by "Do you know the code responsible for this behavior?". Do I know what is responsible for different order of columns in the DataFrame in the particular case of this test?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/pandas-dev/pandas/pull/25224#discussion_r256177741>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOUQ6ufDxF8qxcIyIHxcBpfSfu9SR_ks5vM0LEgaJpZM4attTh>.
> 


@WillAyd I will have to investigate this and get back to you. I'm not entirely too sure how my change affected test_groupby_agg_ohlc_non_first() because my changes deal with dicts and OrderedDicts in the concat function. So, concat must be used within one of the functions that's used inside the test, but the test doesn't seem to be dealing with any dicts. Please let me know if anything I'm saying is wrong, I recently started solving actual code bugs in Pandas and am still learning. I started my contributions with easy issues and now trying to move onto harder ones :)
Yea no worries - your contributions are greatly appreciated.

If it helps here is where the ohlc function is defined. If you trace the underlying function there back you'll see a concat call, hence why this has some impacts

https://github.com/pandas-dev/pandas/blob/4d44a2aff08c3075d9472e361bd7448a0472494e/pandas/core/groupby/groupby.py#L1311
@WillAyd Thank you. I will be looking into it and will update when I find something.
@WillAyd I've been trying to figure out how to solve the problem with test_groupby_agg_ohlc_non_first() but I'm stuck. Python 3.6 and after, .agg(['sum', 'ohlc']) maintains the order "sum -> ohlc", .agg(['ohlc', 'sum']) maintains the order "ohlc -> sum". When doing .agg(['ohlc', 'sum']), it passes the test in all the versions. This is due to my recent concat change and I traced the call and get why it happens, but I don't know how to solve the issue in this test, without affecting other tests negatively.
If that’s the case I think it’s preferable to change the test rather than add the compat condition. Same thing at the end of the day just less clutter 

Sent from my iPhone

> On Feb 13, 2019, at 11:29 PM, Alexander Ponomaroff <notifications@github.com> wrote:
> 
> @WillAyd I've been trying to figure out how to solve the problem with test_groupby_agg_ohlc_non_first() but I'm stuck. Python 3.6 and after, .agg(['sum', 'ohlc']) maintains the order "sum -> ohlc", .agg(['ohlc', 'sum']) maintains the order "ohlc -> sum". When doing .agg(['ohlc', 'sum']), it passes the test in all the versions. This is due to my recent concat change and I traced the call and get why it happens, but I don't know how to solve the issue in this test, without affecting other tests negatively.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

@WillAyd But the result of .agg(['sum', 'ohlc']) will be different based on the version. So without checking for the version with compat and without changing the order in agg, how can the test be rewritten?
> When doing .agg(['ohlc', 'sum']), it passes the test in all the versions

My point was to just change the test to this instead of doing the compat. Should work in all versions then, no?
@WillAyd I did that in one of my commits, however I realized that the whole point of the test was to test ohlc when it's not passed in first into the agg function. There used to be an error when ohlc wasn't passed in first, but you fixed that. So the test was created to ensure that everything will still work even if ohlc isn't first, so by changing it to agg(['ohlc', 'sum']) will kind of defeat the purpose of the test, because ohlc is first. If changing it to agg(['ohlc', 'sum']) is good with you, I will do it, but I just wanted to bring this up before I do it.
@jreback Why am I failing these tests now? Does it have anything to do with changes happened in the last 2 days?
make sure to merge master
these were just fixed up today
@jreback I did merge master, right before I pushed my changes. It says that everything is up to date.
these look related to your changes
run the tests locally and see
@jreback All tests are green now. 

Looks like the previous errors were resolved by themselves. When I ran the test locally, I did not get them.
@jreback Fixed requested changes, all tests are green.
Sounds good.

On Tue, Mar 12, 2019 at 12:46 PM William Ayd <notifications@github.com>
wrote:

> *@WillAyd* commented on this pull request.
> ------------------------------
>
> In pandas/tests/groupby/test_groupby.py
> <https://github.com/pandas-dev/pandas/pull/25224#discussion_r264805394>:
>
> > @@ -1690,14 +1690,11 @@ def test_groupby_agg_ohlc_non_first():
>          [1, 1, 1, 1, 1],
>          [1, 1, 1, 1, 1]
>      ], columns=pd.MultiIndex.from_tuples((
> -        ('foo', 'ohlc', 'open'), ('foo', 'ohlc', 'high'),
> -        ('foo', 'ohlc', 'low'), ('foo', 'ohlc', 'close'),
> -        ('foo', 'sum', 'foo'))), index=pd.date_range(
> +        ('foo', 'sum', 'foo'), ('foo', 'ohlc', 'open'),
>
> The latter. So this has implications to ordering which previously would
> have been non-deterministic in Py35.
>
> So I think it might make sense as a follow up to use a OrderedDict and
> make better guarantees about the returned column order, but don't think
> that should hold this one up
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/25224#discussion_r264805394>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIg6jzfNZb8_yQW9MXzmjZPYWaO0Cks5vV-gIgaJpZM4attTh>
> .
>

Thank you @WillAyd and @TomAugspurger for the feedback and the help on this one. @jreback could you take a look if there are any additional changes that need to take place here?
merging if any additional whatsnew is needed, can do in #25692