-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
BUG: 27453 right merge order #31278
BUG: 27453 right merge order #31278
Conversation
4cf9afe
to
7ea1192
Compare
lgtm. not sure of the 1.0 milestone at this point but let's see what @TomAugspurger and @jreback think |
doc/source/whatsnew/v1.0.0.rst
Outdated
@@ -1156,6 +1183,7 @@ Reshaping | |||
- Bug in :func:`merge_asof` merging on a tz-aware ``left_index`` and ``right_on`` a tz-aware column (:issue:`29864`) | |||
- Improved error message and docstring in :func:`cut` and :func:`qcut` when `labels=True` (:issue:`13318`) | |||
- Bug in missing `fill_na` parameter to :meth:`DataFrame.unstack` with list of levels (:issue:`30740`) | |||
- :meth:`DataFrame.merge` now preserves right frame's row order when executing a right merge (:issue:`27453`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this had been requested to be removed in the original PR (will wait for a decision as to whether to include this in v1 before putting it through another round of CI)
Is this fixing a regression in 0.25? Otherwise would prefer to keep it for 1.0.1
… On Jan 24, 2020, at 10:51, William Ayd ***@***.***> wrote:
lgtm. not sure of the 1.0 milestone at this point but let's see what @TomAugspurger and @jreback think
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@TomAugspurger as in, is it something that was working in 0.25 but isn't working on master? If so, then no, it's not a regression in 0.25. Should I make a v1.0.1.rst whatsnew file then? |
no there is already a PR for that |
3da1145
to
cba4abb
Compare
@TomAugspurger sure, whatsnew entry added in v1.0.1 |
I think this should be 1.1 at this point? AFAIK not a regression |
@WillAyd Sure, have moved the whatsnew entry to v1.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
||
@pytest.mark.parametrize("how", ["left", "right"]) | ||
def test_merge_preserves_row_order(how): | ||
# GH 27453 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the example from the OP (I would rather use that one exactly)
correct the imports broken commit with a bunch of print statements and comments add test for left merge swap left and right keys when how == "right" correct old test: right-merge row order is now the same as the right df clean up spacing and delete temp code add whatsnew replace .from_records with default constructor add GH issue # to tests revert commit ed54bec change logic to swap left and right if how==right clean formatting rename vars and add comment for clarity combine tests into one update whatsnew Update doc/source/whatsnew/v1.0.0.rst Co-Authored-By: William Ayd <[email protected]> add before and after examples linting cleanup changes requested by jreback update docs
86c1b52
to
bab654e
Compare
ping :) |
pandas/core/reshape/merge.py
Outdated
@@ -1847,7 +1847,7 @@ def _right_outer_join(x, y, max_groups): | |||
return left_indexer, right_indexer | |||
|
|||
|
|||
def _factorize_keys(lk, rk, sort=True): | |||
def _factorize_keys(lk, rk, sort=True, how="inner"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a doc-string and typing here
expected["B"] = df1["B"] | ||
tm.assert_frame_equal(result, expected) | ||
|
||
left_df = pd.DataFrame({"colors": ["blue", "red"]}, index=pd.Index([0, 1])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either reduce the examples here (they look like 3x of the same one), or parameterize over them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. some very minor doc comments, ping on green.
doc/source/whatsnew/v1.1.0.rst
Outdated
left_df | ||
right_df | ||
|
||
*pandas 1.0.x* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to prior behavior
doc/source/whatsnew/v1.1.0.rst
Outdated
0 pig 11 | ||
1 quetzal 80 | ||
|
||
*pandas 1.1.0* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current behavior
@jreback thanks - done |
thanks @MarcoGorelli for taking this over the line! |
Resurrection of #27762
I fixed the NameError in the test, and also changed the example given in the whatsnew entry (I couldn't see a difference between the outputs of 0.25.x and 1.0.0 in the original)
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff