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

Checks for left_index and right_index merge parameters #14434

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@ivallesp
Contributor

ivallesp commented Oct 16, 2016

Hi,

I just committed an error when I was doing an analysis using pandas and this motivated me to implement two checks which in my opinion are necessary.

I was trying to perform a merge and I confused the parameters "left_on" and "right_on" for "left_index" and "right_index". I ran the code and it did not raised me any error. It produced a table which seem to be fine in terms of shape. I suspect that what happened is that the tables got merged by the index of the data frame. I think it would be a great idea to check if right_index and left_index are of type bool, if not, raise an error. This way we will avoid that more people got the same error as mine :D.

Tests passed

Thanks!

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 16, 2016

Current coverage is 85.25% (diff: 100%)

Merging #14434 into master will increase coverage by <.01%

@@             master     #14434   diff @@
==========================================
  Files           140        140          
  Lines         50631      50635     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43166      43171     +5   
+ Misses         7465       7464     -1   
  Partials          0          0          

Powered by Codecov. Last update c31ea34...e18b7c9

codecov-io commented Oct 16, 2016

Current coverage is 85.25% (diff: 100%)

Merging #14434 into master will increase coverage by <.01%

@@             master     #14434   diff @@
==========================================
  Files           140        140          
  Lines         50631      50635     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43166      43171     +5   
+ Misses         7465       7464     -1   
  Partials          0          0          

Powered by Codecov. Last update c31ea34...e18b7c9

Show outdated Hide outdated pandas/tools/merge.py
@@ -471,6 +471,14 @@ def __init__(self, left, right, how='inner', on=None,
raise ValueError(
'can not merge DataFrame with instance of '
'type {0}'.format(type(right)))
if not isinstance(left_index, bool):

This comment has been minimized.

@sinhrks

sinhrks Oct 16, 2016

Member

pls use pandas.api.types.is_bool.

@sinhrks

sinhrks Oct 16, 2016

Member

pls use pandas.api.types.is_bool.

This comment has been minimized.

@ivallesp

ivallesp Oct 17, 2016

Contributor

done

@ivallesp

ivallesp Oct 17, 2016

Contributor

done

Show outdated Hide outdated pandas/tools/merge.py
@@ -471,6 +471,14 @@ def __init__(self, left, right, how='inner', on=None,
raise ValueError(
'can not merge DataFrame with instance of '
'type {0}'.format(type(right)))
if not isinstance(left_index, bool):
raise ValueError(
'left_index parameter must be of type bool, not '

This comment has been minimized.

@sinhrks

sinhrks Oct 16, 2016

Member

can u add tests to check expected error is raised?

@sinhrks

sinhrks Oct 16, 2016

Member

can u add tests to check expected error is raised?

This comment has been minimized.

@ivallesp

ivallesp Oct 17, 2016

Contributor

done, thanks!

@ivallesp

ivallesp Oct 17, 2016

Contributor

done, thanks!

@sinhrks sinhrks added this to the 0.19.1 milestone Oct 16, 2016

@sinhrks sinhrks added the Reshaping label Oct 16, 2016

Show outdated Hide outdated pandas/tests/test_categorical.py
@@ -4049,6 +4049,24 @@ def test_merge(self):
result = pd.merge(cleft, cright, how='left', left_on='b', right_on='c')
tm.assert_frame_equal(result, expected)
# params left_index right_index checks
self.assertRaises(ValueError,

This comment has been minimized.

@jreback

jreback Oct 17, 2016

Contributor

wrong file. should go in tools/tests/test_merge.py

@jreback

jreback Oct 17, 2016

Contributor

wrong file. should go in tools/tests/test_merge.py

This comment has been minimized.

@ivallesp

ivallesp Oct 17, 2016

Contributor

thanks!. Updated

@ivallesp

ivallesp Oct 17, 2016

Contributor

thanks!. Updated

@ivallesp

This comment has been minimized.

Show comment
Hide comment
@ivallesp

ivallesp Oct 17, 2016

Contributor

Updated!

Contributor

ivallesp commented Oct 17, 2016

Updated!

@ivallesp

This comment has been minimized.

Show comment
Hide comment
@ivallesp

ivallesp Oct 18, 2016

Contributor

Is there still any change to do? Thanks!

Contributor

ivallesp commented Oct 18, 2016

Is there still any change to do? Thanks!

@@ -109,6 +109,15 @@ def test_merge_misspecified(self):
self.assertRaises(ValueError, merge, self.df, self.df2,
left_on=['key1'], right_on=['key1', 'key2'])
def test_index_and_on_parameters_confusion(self):
self.assertRaises(ValueError, merge, self.df, self.df2, how='left',

This comment has been minimized.

@jreback

jreback Oct 19, 2016

Contributor

add the github issue reference as a comment

@jreback

jreback Oct 19, 2016

Contributor

add the github issue reference as a comment

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

Can you address this one?

@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

Can you address this one?

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 19, 2016

Contributor

pls add a whatsnew entry in 0.19.1. ping on green.

Contributor

jreback commented Oct 19, 2016

pls add a whatsnew entry in 0.19.1. ping on green.

@ivallesp

This comment has been minimized.

Show comment
Hide comment
@ivallesp

ivallesp Oct 19, 2016

Contributor

Done!

Contributor

ivallesp commented Oct 19, 2016

Done!

@ivallesp

This comment has been minimized.

Show comment
Hide comment
@ivallesp

ivallesp Oct 20, 2016

Contributor

@jreback green

Contributor

ivallesp commented Oct 20, 2016

@jreback green

@jreback jreback closed this in 2d3a739 Oct 20, 2016

@jorisvandenbossche

@ivallesp two small comments, looks good!

New features
~~~~~~~~~~~~
- Add checks to assure that left_index and right_index are of type bool

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

Can you move this to the bug fixes section? (I think that it is rather a bug that it accepted a wrong value, than a new feature that it now checks that)

@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

Can you move this to the bug fixes section? (I think that it is rather a bug that it accepted a wrong value, than a new feature that it now checks that)

@@ -109,6 +109,15 @@ def test_merge_misspecified(self):
self.assertRaises(ValueError, merge, self.df, self.df2,
left_on=['key1'], right_on=['key1', 'key2'])
def test_index_and_on_parameters_confusion(self):
self.assertRaises(ValueError, merge, self.df, self.df2, how='left',

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

Can you address this one?

@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

Can you address this one?

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

@ivallesp No matter, @jreback already merged and addressed those comments!

Member

jorisvandenbossche commented Oct 20, 2016

@ivallesp No matter, @jreback already merged and addressed those comments!

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 20, 2016

Contributor

@jorisvandenbossche was your 2nd one the comment reference in the tests? (decided not a big deal)

Contributor

jreback commented Oct 20, 2016

@jorisvandenbossche was your 2nd one the comment reference in the tests? (decided not a big deal)

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

yep, but indeed not a big deal

Member

jorisvandenbossche commented Oct 20, 2016

yep, but indeed not a big deal

tworec pushed a commit to RTBHOUSE/pandas that referenced this pull request Oct 21, 2016

ERR: Checks for left_index and right_index merge parameters
Author: Iván Vallés Pérez <ivanvallesperez@gmail.com>

Closes #14434 from ivallesp/add-check-for-merge-indices and squashes the following commits:

e18b7c9 [Iván Vallés Pérez] Add some checks for assuring that the left_index and right_index parameters have correct types. Tests added.

jorisvandenbossche added a commit that referenced this pull request Nov 1, 2016

[Backport #14434] ERR: Checks for left_index and right_index merge pa…
…rameters

Author: Iván Vallés Pérez <ivanvallesperez@gmail.com>

Closes #14434 from ivallesp/add-check-for-merge-indices and squashes the following commits:

e18b7c9 [Iván Vallés Pérez] Add some checks for assuring that the left_index and right_index parameters have correct types. Tests added.

(cherry picked from commit 2d3a739)

amolkahat added a commit to amolkahat/pandas that referenced this pull request Nov 26, 2016

ERR: Checks for left_index and right_index merge parameters
Author: Iván Vallés Pérez <ivanvallesperez@gmail.com>

Closes #14434 from ivallesp/add-check-for-merge-indices and squashes the following commits:

e18b7c9 [Iván Vallés Pérez] Add some checks for assuring that the left_index and right_index parameters have correct types. Tests added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment