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.

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
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.23.1.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ New features
~~~~~~~~~~~~

- :meth:`Index.droplevel` is now implemented also for flat indexes, for compatibility with MultiIndex (:issue:`21115`)
- :func: merge now allows a ``DataFrame`` and a ``Series`` with a name as inputs (:issue:`21220`)


.. _whatsnew_0231.deprecations:
Expand Down
24 changes: 16 additions & 8 deletions pandas/core/reshape/merge.py
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,8 @@ 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):
left = validate_operand(left)
right = validate_operand(right)
self.left = self.orig_left = left
self.right = self.orig_right = right
self.how = how
Expand All @@ -518,13 +520,6 @@ def __init__(self, left, right, how='inner', on=None,
raise ValueError(
'indicator option can only accept boolean or string arguments')

if not isinstance(left, DataFrame):
raise ValueError('can not merge DataFrame with instance of '
'type {left}'.format(left=type(left)))
if not isinstance(right, DataFrame):
raise ValueError('can not merge DataFrame with instance of '
'type {right}'.format(right=type(right)))

if not is_bool(left_index):
raise ValueError(
'left_index parameter must be of type bool, not '
Expand Down Expand Up @@ -1639,3 +1634,16 @@ def _should_fill(lname, rname):

def _any(x):
return x is not None and com._any_not_none(*x)


def validate_operand(obj):
if isinstance(obj, DataFrame):
return obj
elif isinstance(obj, Series):
if obj.name is None:
raise ValueError('Cannot merge a Series without a name')
else:
return obj.to_frame()
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

just raise, and more like

Can only merge Series or DataFrame objects, a {obj} was passed

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 - done - updated the statement and made into a TypeError

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you changed this to TypeError?
I would personally keep it as ValueError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorisvandenbossche Thanks - I thought TypeError is more relevant for the statement below, as the message is telling the user "only objects of type Series or DataFrame can be merged"
Should I revert it back to ValueError?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the one is necessarily much better than the other in this case, so I would personally rather keep it as it was to be conservative.

Copy link
Member

Choose a reason for hiding this comment

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

Although eg concat raises TypeError if the input is not series or dataframe, so maybe TypeError is more consistent with pandas.

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jul 23, 2018

Choose a reason for hiding this comment

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

Ok - not changing then and keeping it (i.e. TypeError)

raise ValueError('Cannot merge a DataFrame or a Series with '
'instance of type {obj}'.format(obj=type(obj)))
5 changes: 4 additions & 1 deletion pandas/tests/reshape/merge/test_join.py
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
39 changes: 39 additions & 0 deletions pandas/tests/reshape/merge/test_merge.py
Original file line number Diff line number Diff line change
Expand Up @@ -1864,3 +1864,42 @@ 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

@pytest.mark.parametrize("on,left_on,right_on,left_index,right_index,nms,nm", [
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 format this so it lines up, its pretty unreadable now

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

(['outer', 'inner'], None, None, False, False,
['outer', 'inner'], 'B'),
(None, None, None, True, True, ['outer', 'inner'],
'B'),
(None, ['outer', 'inner'], None, False, True, None,
'B'),
(None, None, ['outer', 'inner'], True, False, None,
'B'),
(['outer', 'inner'], None, None, False, False,
['outer', 'inner'], None),
(None, None, None, True, True, ['outer', 'inner'],
None),
(None, ['outer', 'inner'], None, False, True, None,
None),
(None, None, ['outer', 'inner'], True, False, None,
None),
])
def test_merge_series(on, left_on, right_on, left_index, right_index, nms, nm):
# 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=nm)
expected = pd.DataFrame({"A": [2, 4], "B": [1, 3]},
index=pd.MultiIndex.from_product([['a', 'b'], [1]],
names=nms))
if nm is not None:
result = pd.merge(a, b, on=on, left_on=left_on, right_on=right_on,
left_index=left_index, right_index=right_index)
tm.assert_frame_equal(result, expected)
else:
with tm.assert_raises_regex(ValueError, 'a Series without a name'):
result = pd.merge(a, b, on=on, left_on=left_on, right_on=right_on,
left_index=left_index, right_index=right_index)