Checks for left_index and right_index merge parameters by ivallesp · Pull Request #14434 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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!
Current coverage is 85.25% (diff: 100%)
@@ 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
| @@ -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): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls use pandas.api.types.is_bool.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -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 ' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u add tests to check expected error is raised?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks!
| @@ -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, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong file. should go in tools/tests/test_merge.py
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!. Updated
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', |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the github issue reference as a comment
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you address this one?
pls add a whatsnew entry in 0.19.1. ping on green.
…meters have correct types. Tests added.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivallesp two small comments, looks good!
| New features |
| ~~~~~~~~~~~~ |
| - Add checks to assure that left_index and right_index are of type bool |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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', |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you address this one?
@jorisvandenbossche was your 2nd one the comment reference in the tests? (decided not a big deal)
yep, but indeed not a big deal
tworec pushed a commit to RTBHOUSE/pandas that referenced this pull request
Author: Iván Vallés Pérez ivanvallesperez@gmail.com
Closes pandas-dev#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 pushed a commit that referenced this pull request
…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)