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

KalyanGokhale · 2018-05-27T15:08:47Z

closes Merge DataFrame and Series using on #21220
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

GH21220

KalyanGokhale · 2018-05-27T15:10:33Z

Not sure if any another changes / functionality changes are required

Also, not sure whether this PR title should go under 'CLN' or 'ENH'?

TomAugspurger · 2018-05-27T18:51:44Z

Looks like we had some tests that asserted this did not work: https://travis-ci.org/pandas-dev/pandas/jobs/384393959#L2444. Can you go through those and see what the original motivation was?

Will also need a release note in 0.24.0

TomAugspurger · 2018-05-27T18:52:02Z

cc @jmmease

TomAugspurger · 2018-05-27T18:53:18Z

On thing to nail down: what's the type of merge(Series, Series)? Do we want merge to always return a DataFrame?

GH21220

KalyanGokhale · 2018-05-28T02:21:45Z

Thanks @TomAugspurger

Looks like we had some tests that asserted this did not work: ... Can you go through those and see what the original motivation was?

test_join_on_fails_with_wrong_object_type inpandas/tests/reshape/merge/test_join.py was failing as it was checking that a ValueError is raised on merging Series with DataFrame - have now edited out the portion for Series from that test

Will also need a release note in 0.24.0

will make changes to release note once the tests run successfully and also there is consensus on whether merge should always return a DataFrame

jorisvandenbossche · 2018-05-28T08:23:27Z

On thing to nail down: what's the type of merge(Series, Series)? Do we want merge to always return a DataFrame?

That would only be a question when merging on the values and not the index I think?
But personally I would go for consistent output of always DataFrame.

We will need to add tests for this as well.

GH21220

codecov · 2018-05-28T13:05:45Z

Codecov Report

Merging #21223 into master will decrease coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master   #21223      +/-   ##
==========================================
- Coverage   91.84%   91.83%   -0.01%     
==========================================
  Files         153      153              
  Lines       49505    49509       +4     
==========================================
+ Hits        45466    45469       +3     
- Misses       4039     4040       +1

Flag	Coverage Δ
#multiple	`90.23% <75%> (-0.01%)`	⬇️
#single	`41.87% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/reshape/merge.py	`94.14% <75%> (-0.12%)`	⬇️

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 1c2844a...b09ffb7. Read the comment docs.

codecov · 2018-05-28T13:05:53Z

Codecov Report

❗ No coverage uploaded for pull request base (master@27ebb3e). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #21223   +/-   ##
=========================================
  Coverage          ?      92%           
=========================================
  Files             ?      168           
  Lines             ?    50603           
  Branches          ?        0           
=========================================
  Hits              ?    46555           
  Misses            ?     4048           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.4% <100%> (?)`
#single	`42.19% <10%> (?)`

Impacted Files	Coverage Δ
pandas/core/frame.py	`97.19% <ø> (ø)`
pandas/core/reshape/merge.py	`94.16% <100%> (ø)`

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 27ebb3e...d099fd6. Read the comment docs.

jreback · 2018-05-28T13:07:33Z

pandas/core/reshape/merge.py

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


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

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)

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

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

jreback · 2018-05-28T13:09:17Z

pandas/tests/reshape/merge/test_merge.py

+                            index=pd.MultiIndex.from_product([['a', 'b'], [1]],
+                            names=['outer', 'inner']))
+
+    # Testing current merge behvaior is as before


these comments are not meaningful pls change them

GH21220

jreback · 2018-05-28T17:29:44Z

pandas/core/reshape/merge.py

+        if isinstance(left, Series):
+            left = left.to_frame()
+        if isinstance(right, Series):
+            right = right.to_frame()


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

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

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

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)

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

jreback · 2018-05-29T00:36:25Z

pandas/tests/reshape/merge/test_merge.py

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


also parameterize this as much as possible.

jreback · 2018-05-29T00:36:39Z

also need a whatsnew note

GH21220

TomAugspurger

This looks good, but the release note should be in 0.24.0 I think.

GH21220

KalyanGokhale · 2018-06-07T16:31:42Z

Thanks @TomAugspurger
Done - moved the whatsnew note to 0.24.0
As this PR was removed from 0.24.0 milestone had earlier added whatsnew note to 0.23.1

GH21220

KalyanGokhale · 2018-06-16T06:22:16Z

Are any additional changes needed on this PR? Thanks

jreback · 2018-06-19T01:14:45Z

doc/source/whatsnew/v0.24.0.txt

@@ -7,6 +7,7 @@ v0.24.0

 New features
 ~~~~~~~~~~~~
+- :func: merge now allows  a ``DataFrame`` and a ``Series`` with a name as inputs (:issue:`21220`)


this won't render, use :func:`merge` . also not clear what the change is from a user POV here

Thanks - updated the comment as:

:func:`merge` now directly allows merge between objects of type ``DataFrame`` and ``Series`` with a name, without need to convert the ``Series`` object into a ``DataFrame`` beforehand (:issue:`21220`)

jreback · 2018-06-19T01:15:58Z

pandas/core/reshape/merge.py

+            raise ValueError('Cannot merge a Series without a name')
+        else:
+            return obj.to_frame()
+    else:


just raise, and more like

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

Thanks - done - updated the statement and made into a TypeError

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

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

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.

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

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

jreback · 2018-06-19T01:16:18Z

pandas/tests/reshape/merge/test_join.py

@@ -230,7 +230,9 @@ def test_join_on_fails_with_different_column_counts(self):

    def test_join_on_fails_with_wrong_object_type(self):


can you parameterize this test

jreback · 2018-06-19T01:17:04Z

pandas/tests/reshape/merge/test_merge.py

@@ -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)
+
+
+@pytest.mark.parametrize("on,left_on,right_on,left_index,right_index,nms,nm", [


can you format this so it lines up, its pretty unreadable now

Thanks - done

GH21220

KalyanGokhale · 2018-06-20T10:58:25Z

@jreback please let me know if any other edits are needed on this one. Thanks.

TomAugspurger · 2018-06-20T12:45:41Z

Could you update the docstring _merge_doc in pandas/core/frame.py to reflect the ability to merge DataFrames and named series? merging.rst also has a few references to joining DataFrame objects, that should be updated to DataFrame or named series.

TomAugspurger · 2018-06-20T12:45:46Z

Looks good otherwise.

GH21220

KalyanGokhale · 2018-06-20T15:29:00Z

@TomAugspurger Thanks - made the updates. Any other edits needed?

TomAugspurger · 2018-06-20T18:55:05Z

Waiting for CI to finish.

…

On Wed, Jun 20, 2018 at 10:29 AM, Kalyan Gokhale ***@***.***> wrote: @TomAugspurger <https://github.com/TomAugspurger> Thanks - made the updates. Any other edits needed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21223 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIq9_4MXB4Hm-4Ppp7n5zLUmuNx_Jks5t-mpFgaJpZM4UPUDU> .

KalyanGokhale · 2018-07-17T12:52:30Z

Any additional work needed on this? Thanks

TomAugspurger · 2018-07-19T11:39:28Z

There's a merge conflict in core/frame.py now. Can you merge in master and resolve that, and let us know if you see that all the CI pass?

If you can't get to fixing the merge conflict, I'll push a fix when I get a chance.

KalyanGokhale · 2018-07-19T14:35:02Z

pandas/core/frame.py


 If joining columns on columns, the DataFrame indexes *will be
 ignored*. Otherwise if joining indexes on indexes or indexes on a column or
 columns, the index will be passed on.

 Parameters
 ----------%s
-right : DataFrame, Series or dict


As of my prior commit 69d8bda initially this only said right : DataFrame, not sure when the part around dict was added. Have removed mention of dict as this PR only ensures that DataFrame and Series can be merged

Have not tested merge with dict - as recently changed local machine - is this needed in this PR?

KalyanGokhale · 2018-07-19T16:03:44Z

@TomAugspurger resolved the merge conflict - all CI tests passed. Any other changes needed? Thanks

jorisvandenbossche

Looks good to me now in general, added 2 minor comments

jorisvandenbossche · 2018-07-23T11:09:19Z

pandas/core/reshape/merge.py

+            raise ValueError('Cannot merge a Series without a name')
+        else:
+            return obj.to_frame()
+    else:


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

jorisvandenbossche · 2018-07-23T11:10:18Z

pandas/tests/reshape/merge/test_join.py

+
+        # GH21220 - merging of Series and DataFrame is now allowed
+        # Edited the test to remove the Series object from test parameters
+        # Also, parameterized the original test


this last line of the comment is not really needed (in the sense of: it is not really useful for future readers of this code, they will see that the test is parametrized)

Thanks - makes sense, will remove the redundant comment - will make the change together with any changes needed for your ValueError/TypeError comment above.

KalyanGokhale · 2018-07-23T16:11:01Z

@jorisvandenbossche made the suggested change - all CI tests passed. Thanks

jorisvandenbossche · 2018-07-23T17:02:25Z

@KalyanGokhale Thanks a lot for this PR!

KalyanGokhale added 2 commits May 27, 2018 20:25

Initial commit

91a3f16

GH21220

Removed some now unrequired edits left

64e6e66

GH21220

TomAugspurger added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label May 27, 2018

TomAugspurger added this to the 0.24.0 milestone May 27, 2018

Edits to test_join

8939fad

GH21220

Minor comment edit to force Travis CI rerun

b09ffb7

GH21220

jreback requested changes May 28, 2018

View reviewed changes

Updated comments in test

1a0e5a2

GH21220

jreback requested changes May 28, 2018

View reviewed changes

jreback requested changes May 29, 2018

View reviewed changes

jreback removed this from the 0.24.0 milestone May 29, 2018

jreback added the Enhancement label May 29, 2018

KalyanGokhale changed the title ~~CLN: Merge DataFrame and Series using on (GH21220)~~ ENH: Merge DataFrame and Series using on (GH21220) May 29, 2018

Validate operand, parametrized tests, whatnew

59b487d

GH21220

TomAugspurger reviewed Jun 7, 2018

View reviewed changes

KalyanGokhale added 2 commits June 7, 2018 21:57

Moved whatsnew to 0.24.0

0d14cc2

GH21220

Moved whatsnew to 0.24.0

12da2ce

GH21220

Updated comment in test_join

2d689ad

GH21220

jreback requested changes Jun 19, 2018

View reviewed changes

KalyanGokhale added 3 commits June 19, 2018 20:39

Linting, parameterizing test, whatsnew, raising

687568b

GH21220

Updated test_join

7ed9688

GH21220

Minor update to whatsnew to force TravisCI rebuild

2ce56e7

GH21220

KalyanGokhale added 4 commits June 20, 2018 20:38

Updating documentation

ac9d5a1

GH21220

Resolving conflicts for merging.rst

fdf324c

GH21220

Edits to rst

e896a91

GH21220

Whatsnew

69d8bda

GH21220

jorisvandenbossche added this to the 0.24.0 milestone Jun 20, 2018

Merge branch 'master' into GH21220

f7b333f

KalyanGokhale commented Jul 19, 2018

View reviewed changes

jorisvandenbossche reviewed Jul 23, 2018

View reviewed changes

KalyanGokhale added 2 commits July 23, 2018 19:29

Update test_join.py

c4c988c

Force CI tests

d099fd6

jorisvandenbossche approved these changes Jul 23, 2018

View reviewed changes

jorisvandenbossche merged commit b975455 into pandas-dev:master Jul 23, 2018

KalyanGokhale deleted the GH21220 branch July 23, 2018 17:03

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

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

acfc616

mroeschke mentioned this pull request Oct 21, 2018

Why does df.merge(x) not work when x is a pd.Series? #23250

Closed

		@@ -230,7 +230,9 @@ def test_join_on_fails_with_different_column_counts(self):

		def test_join_on_fails_with_wrong_object_type(self):

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

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

Conversation

KalyanGokhale commented May 27, 2018 • edited Loading

KalyanGokhale commented May 27, 2018

TomAugspurger commented May 27, 2018

TomAugspurger commented May 27, 2018

TomAugspurger commented May 27, 2018

KalyanGokhale commented May 28, 2018

jorisvandenbossche commented May 28, 2018 • edited Loading

codecov bot commented May 28, 2018

Codecov Report

codecov bot commented May 28, 2018 • edited Loading

Codecov Report

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

KalyanGokhale May 28, 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

jreback commented May 29, 2018

TomAugspurger left a comment

Choose a reason for hiding this comment

KalyanGokhale commented Jun 7, 2018 • edited Loading

KalyanGokhale 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

KalyanGokhale Jul 23, 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

KalyanGokhale commented Jun 20, 2018

TomAugspurger commented Jun 20, 2018

TomAugspurger commented Jun 20, 2018

KalyanGokhale commented Jun 20, 2018

TomAugspurger commented Jun 20, 2018 via email

KalyanGokhale commented Jul 17, 2018

TomAugspurger commented Jul 19, 2018

KalyanGokhale Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalyanGokhale commented Jul 19, 2018

jorisvandenbossche left a 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

KalyanGokhale commented Jul 23, 2018

jorisvandenbossche commented Jul 23, 2018

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

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

KalyanGokhale commented May 27, 2018 •

edited

Loading

jorisvandenbossche commented May 28, 2018 •

edited

Loading

codecov bot commented May 28, 2018 •

edited

Loading

KalyanGokhale May 28, 2018 •

edited

Loading

KalyanGokhale commented Jun 7, 2018 •

edited

Loading

KalyanGokhale Jul 23, 2018 •

edited

Loading

KalyanGokhale Jul 19, 2018 •

edited

Loading