Skip to content

ENH: Merge DataFrame and Series using on (GH21220) #21223

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

Merged
merged 19 commits into from
Jul 23, 2018
Merged

ENH: Merge DataFrame and Series using on (GH21220) #21223

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
91a3f16
Initial commit
KalyanGokhale May 27, 2018
64e6e66
Removed some now unrequired edits left
KalyanGokhale May 27, 2018
8939fad
Edits to test_join
KalyanGokhale May 28, 2018
b09ffb7
Minor comment edit to force Travis CI rerun
KalyanGokhale May 28, 2018
1a0e5a2
Updated comments in test
KalyanGokhale May 28, 2018
59b487d
Validate operand, parametrized tests, whatnew
KalyanGokhale May 31, 2018
0d14cc2
Moved whatsnew to 0.24.0
KalyanGokhale Jun 7, 2018
12da2ce
Moved whatsnew to 0.24.0
KalyanGokhale Jun 7, 2018
2d689ad
Updated comment in test_join
KalyanGokhale Jun 15, 2018
687568b
Linting, parameterizing test, whatsnew, raising
KalyanGokhale Jun 19, 2018
7ed9688
Updated test_join
KalyanGokhale Jun 19, 2018
2ce56e7
Minor update to whatsnew to force TravisCI rebuild
KalyanGokhale Jun 20, 2018
ac9d5a1
Updating documentation
KalyanGokhale Jun 20, 2018
fdf324c
Resolving conflicts for merging.rst
KalyanGokhale Jun 20, 2018
e896a91
Edits to rst
KalyanGokhale Jun 20, 2018
69d8bda
Whatsnew
KalyanGokhale Jun 20, 2018
f7b333f
Merge branch 'master' into GH21220
KalyanGokhale Jul 19, 2018
c4c988c
Update test_join.py
KalyanGokhale Jul 23, 2018
d099fd6
Force CI tests
KalyanGokhale Jul 23, 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
6 changes: 5 additions & 1 deletion pandas/core/reshape/merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import pandas.compat as compat

from pandas import (Categorical, DataFrame,
Index, MultiIndex, Timedelta)
Index, MultiIndex, Timedelta, Series)
from pandas.core.arrays.categorical import _recode_for_categories
from pandas.core.frame import _merge_doc
from pandas.core.dtypes.common import (
Expand Down Expand Up @@ -492,6 +492,10 @@ def __init__(self, left, right, how='inner', on=None,
left_index=False, right_index=False, sort=True,
suffixes=('_x', '_y'), copy=True, indicator=False,
validate=None):
if isinstance(left, Series):
Copy link
Contributor

Choose a reason for hiding this comment

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

if u are actually going to do this
then you need to strip a ton of logic from below

Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify this comment? What logic would need to be stripped?

Currently, this just raises when you pass a Series (see line 525 a bit below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks both - will make edits to this based on @jreback 's clarification

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm on the code removal, we do extension promotion in concat, but nothing here.

left = left.to_frame()
if isinstance(right, Series):
right = right.to_frame()
Copy link
Contributor

Choose a reason for hiding this comment

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

need to validate that the input is a Series or Dataframe and nothing else

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale May 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks - Had initially addeded these validations at 525 and 528 and also before 541 (if memory serves right) on my local machine - but then it fails at 571...

Thus after trying to fix the same issue (checking if input is Series or DataFrame) at multiple places, the current change seemed like the most logical fix as we are changing the type of incoming Series object to DataFrame and then avoiding any susequent changes to the code....

However open to suggestions...

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 ok, but what I mean is validation that you have a Series/DataFrame and nothing else. (and a test to go with that), something like

left = validate_operand(left)
right = validate_operand(right)


......

def validate_operand(obj):
     if isinstance(obj, DataFrame): 
         return obj
     elif isinstance(obj, Series):
        return obj.to_frame()
     raise ValueError(.....)

Copy link
Contributor

Choose a reason for hiding this comment

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

also need to test this with left_index=True, right_index=True (which should be fine). And a test that a Series w/o a name raises (it will convert just fine, but then you would have to specify the on in a funny way)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review - done.
also raises a ValueError for Series w/o a name

self.left = self.orig_left = left
self.right = self.orig_right = right
self.how = how
Expand Down
5 changes: 4 additions & 1 deletion pandas/tests/reshape/merge/test_join.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,10 @@ def test_join_on_fails_with_different_column_counts(self):

def test_join_on_fails_with_wrong_object_type(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you parameterize this test

# GH12081
wrongly_typed = [Series([0, 1]), 2, 'str', None, np.array([0, 1])]
""" GH21220 - merging of Series and DataFrame is now allowed
Edited the test to remove the Series object from 'wrongly_typed'
"""
wrongly_typed = [2, 'str', None, np.array([0, 1])]
df = DataFrame({'a': [1, 1]})

for obj in wrongly_typed:
Expand Down
21 changes: 21 additions & 0 deletions pandas/tests/reshape/merge/test_merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1864,3 +1864,24 @@ def test_merge_index_types(index):
OrderedDict([('left_data', [1, 2]), ('right_data', [1.0, 2.0])]),
index=index)
assert_frame_equal(result, expected)


Copy link
Contributor

Choose a reason for hiding this comment

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

also parameterize this as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

def test_merge_series():
# GH 21220
a = pd.DataFrame({"A": [1, 2, 3, 4]},
index=pd.MultiIndex.from_product([['a', 'b'], [0, 1]],
names=['outer', 'inner']))
b = pd.Series([1, 2, 3, 4],
index=pd.MultiIndex.from_product([['a', 'b'], [1, 2]],
names=['outer', 'inner']), name='B')
expected = pd.DataFrame({"A": [2, 4], "B": [1, 3]},
index=pd.MultiIndex.from_product([['a', 'b'], [1]],
names=['outer', 'inner']))

# Testing current merge behvaior is as before
Copy link
Contributor

Choose a reason for hiding this comment

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

these comments are not meaningful pls change them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

result = pd.merge(a, b.to_frame(), on=['outer', 'inner'])
tm.assert_frame_equal(result, expected)

# Testing changed merge behvaior is as expected
result = pd.merge(a, b, on=['outer', 'inner'])
tm.assert_frame_equal(result, expected)