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

DOC/ERR: better error message on no common merge keys #19427

Merged
merged 1 commit into from Feb 6, 2018

Conversation

Projects
None yet
3 participants
@swyoon
Contributor

swyoon commented Jan 28, 2018

  • [o] closes #19391
  • [o] tests passed
  • [o] passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • Let MergeError emit values keyword arguments
  • Add DataFrame.join on See also section of pandas.merge
@swyoon

This comment has been minimized.

Contributor

swyoon commented Jan 28, 2018

It seems that the failed test has nothing to do with the content of the commit though..

@codecov

This comment has been minimized.

codecov bot commented Jan 30, 2018

Codecov Report

Merging #19427 into master will decrease coverage by 1.63%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19427      +/-   ##
==========================================
- Coverage    91.6%   89.96%   -1.64%     
==========================================
  Files         150      150              
  Lines       48750    48726      -24     
==========================================
- Hits        44657    43837     -820     
- Misses       4093     4889     +796
Flag Coverage Δ
#multiple 89.96% <100%> (-0.01%) ⬇️
#single ?
Impacted Files Coverage Δ
pandas/core/frame.py 97.42% <ø> (-0.01%) ⬇️
pandas/core/reshape/merge.py 94.21% <100%> (-0.01%) ⬇️
pandas/core/computation/pytables.py 62.5% <0%> (-29.88%) ⬇️
pandas/io/pytables.py 64.17% <0%> (-28.28%) ⬇️
pandas/io/feather_format.py 71.42% <0%> (-14.29%) ⬇️
pandas/core/computation/common.py 78.57% <0%> (-7.15%) ⬇️
pandas/core/computation/expr.py 84.27% <0%> (-4.39%) ⬇️
pandas/io/formats/console.py 66.66% <0%> (-3.04%) ⬇️
pandas/io/clipboard/clipboards.py 29.88% <0%> (-2.3%) ⬇️
pandas/io/formats/printing.py 87.61% <0%> (-1.77%) ⬇️
... and 35 more

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 54f1b3e...e4c5b58. Read the comment docs.

@TomAugspurger

This comment has been minimized.

Contributor

TomAugspurger commented Jan 30, 2018

Can you add a test for this, and a release note?

What's the output look like for large DataFrames? Does the repr truncation work correctly?

@@ -233,7 +233,7 @@
--------
merge_ordered
merge_asof
DataFrame.join : For index-based merge operations

This comment has been minimized.

@TomAugspurger

TomAugspurger Jan 30, 2018

Contributor

I think you can remove the explanation, everything other than the name. I'm not sure what numpydoc does with it, but those are turned into hyperlinks for the HTML docs.

@swyoon

This comment has been minimized.

Contributor

swyoon commented Jan 31, 2018

Release note added, see also modified.
I believe my contribution is about the error message. Should the error message be included in the test?
It seems that there already exists a test for a merge with no common keys.
I don't get what you said about the large DataFrames. Could you explain a bit more?

@TomAugspurger

This comment has been minimized.

Contributor

TomAugspurger commented Jan 31, 2018

I believe my contribution is about the error message. Should the error message be included in the test?

Yes, see pandas.util.testing.assert_raises_regex for how to do it. You'll want to do a few different tests and assert that the correct information is always in the exception message.

Ignore my comment about large dataframes I think.

@@ -1028,7 +1028,14 @@ def _validate_specification(self):
common_cols = self.left.columns.intersection(
self.right.columns)
if len(common_cols) == 0:
raise MergeError('No common columns to perform merge on')
raise MergeError('No common columns to perform merge on. '
'Merge options: right_on={ron}, '

This comment has been minimized.

@TomAugspurger

TomAugspurger Jan 31, 2018

Contributor

Can you change the order to be left_on, right_on, left_index, right_index, to match the order in the signature?

This comment has been minimized.

@swyoon

swyoon Jan 31, 2018

Contributor

Oh sure I can. will do it.

This comment has been minimized.

@jreback

jreback Feb 1, 2018

Contributor

to make this more readable you can break it like

raise MergeError(
      'No common ......'   

)
@@ -1028,7 +1028,14 @@ def _validate_specification(self):
common_cols = self.left.columns.intersection(
self.right.columns)
if len(common_cols) == 0:
raise MergeError('No common columns to perform merge on')
raise MergeError('No common columns to perform merge on. '
'Merge options: right_on={ron}, '

This comment has been minimized.

@jreback

jreback Feb 1, 2018

Contributor

to make this more readable you can break it like

raise MergeError(
      'No common ......'   

)
'left_on={lon}, right_index={ridx}, '
'left_index={lidx}'
.format(ron=self.right_on,
lon=self.left_on,

This comment has been minimized.

@jreback

jreback Feb 1, 2018

Contributor

can you update the test for this (it prob just raises) and match the error message with assert_raises_regex

@jreback

This comment has been minimized.

Contributor

jreback commented Feb 6, 2018

can you rebase and update

@swyoon

This comment has been minimized.

Contributor

swyoon commented Feb 6, 2018

The line break and the order of options are modified.
Added a check for error message by assert_raises_regex.

DOC/ERR: better error message on no common merge keys
- Let MergeError emit values keyword arguments.
- Add DataFrame.join on 'See also' section of
  pandas.merge.
- Add an item in whatsnew
- Update test_no_overlap_more_informative_error
  to check the message from MergeError

@jreback jreback added this to the 0.23.0 milestone Feb 6, 2018

@jreback

jreback approved these changes Feb 6, 2018

@jreback jreback merged commit d5eead6 into pandas-dev:master Feb 6, 2018

1 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Contributor

jreback commented Feb 6, 2018

thanks @swyoon

@swyoon

This comment has been minimized.

Contributor

swyoon commented Feb 6, 2018

thanks for kind reviews! @jreback @TomAugspurger

@swyoon swyoon deleted the swyoon:swyoon branch Feb 6, 2018

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment