BUG: Raise ValueError for non numerical join columns in merge_asof #34488

phofl · 2020-05-30T19:57:50Z

closes merge_asof() returns cryptic error with bad argument types #29130
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

I checked for dtype object. Normally, I would try to check for allowed dtypes, but that would require a lot of dtype checks instead of only one. I hope object dtype is sufficient to exclude

pandas/core/reshape/merge.py

dsaxton · 2020-05-30T21:19:55Z

pandas/core/reshape/merge.py

+        if is_object_dtype(right_values):
+            raise ValueError("Right input is not from numerical dtype")
+
+        if is_object_dtype(left_values.dtype):


Should this be loosened to include other non-numeric dtypes for which this should raise?

There exist tests for Timedelta and Datetime DataFrames, so this is expected to work I think?. Is there anything you would like to exclude?

You might be right, was just curious if this was indeed capturing all the "non-numeric" cases

pandas/tests/reshape/merge/test_merge_asof.py

pep8speaks · 2020-05-30T21:47:36Z

Hello @phofl! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-01-05 18:33:30 UTC

pandas/core/reshape/merge.py

doc/source/whatsnew/v1.1.0.rst

pandas/core/reshape/merge.py

phofl · 2020-06-01T15:19:16Z

@jreback

I think I found a solution. I moved it as you requested to the overwritten _get_merge_keys function. We can only check the on columns, not the by columns for dtype object. So I take the length of the by columns and check all following columns. The check for equal dtypes is performed a few lines before that.

jreback · 2020-06-01T16:00:25Z

pandas/core/reshape/merge.py

@@ -1661,6 +1663,13 @@ def _get_merge_keys(self):
                    )
                raise MergeError(msg)

+            if i >= len_by_cols:


no, we don't want to do this.

put your changes in the super call to _get_merge_keys. just do it upfront (that is what was not clear before).

I think you mean _maybe_coerce_merge_keys then? The dtype validation for the regular merge is done there.

The _get_merge_keys function I changed is already from _AsOfMerge, so I could not overwrite this again. This function overwrites the original _get_merge_keys function and calls the super() function before checking if both join cols have the same dtype

dsaxton · 2020-09-16T01:00:45Z

@phofl Can you merge master to fix conflicts?

jreback · 2020-12-29T20:40:42Z

@phofl can you merge master and move the note to 1.3

� Conflicts: � doc/source/whatsnew/v1.2.0.rst

phofl · 2020-12-29T21:06:23Z

Done

jreback · 2020-12-29T23:42:05Z

pandas/core/reshape/merge.py

@@ -1798,6 +1799,24 @@ def _get_merge_keys(self):

        return left_join_keys, right_join_keys, join_names

+    def _maybe_coerce_merge_keys(self):


why is this not just a part of _validate_specification? seems like re-inventing the wheel here

Because this is way easier after knowing the join keys than before. But I think we could probably move this into validate_specification

yeah would really like it to be part of the main check

� Conflicts: � doc/source/whatsnew/v1.3.0.rst

jreback · 2021-01-03T16:41:43Z

pandas/core/reshape/merge.py

@@ -1798,6 +1799,24 @@ def _get_merge_keys(self):

        return left_join_keys, right_join_keys, join_names

+    def _maybe_coerce_merge_keys(self):


yeah would really like it to be part of the main check

� Conflicts: � doc/source/whatsnew/v1.3.0.rst

pandas/core/reshape/merge.py

jreback · 2021-01-05T01:58:48Z

pandas/core/reshape/merge.py

+            else self.right.index.dtype
+        )
+        if is_object_dtype(lo_dtype) or is_object_dtype(ro_dtype):
+            raise ValueError(


can you make this a MergeError (as everthing else is)

Done, issue mentioned ValueError, but MergeError is probably better

jreback · 2021-01-05T01:58:58Z

pandas/tests/reshape/merge/test_merge_asof.py


        left = pd.DataFrame({"left_val": [1, 5, 10], "a": ["a", "b", "c"]})
        right = pd.DataFrame({"right_val": [1, 2, 3, 6, 7], "a": [1, 2, 3, 6, 7]})

-        with pytest.raises(MergeError, match=msg):
+        with pytest.raises(ValueError, match=msg):


revert (see above)

jreback · 2021-01-06T00:16:32Z

thanks @phofl

…andas-dev#34488)

paulg-quantinno · 2021-07-23T18:51:23Z

Hello, thought I'd add here a bug(?) that happened with the above change that was just released with pandas 1.3.0. Seems the "GH#29130 Check that merge keys do not have dtype object" makes an assumption that a merge key is not in the index, so the "self.right[self.right_on[0]].dtype" fails when self.right_on[0] is not a column of the dataframe (and equivalent for the left_on side). Perhaps those fields should never be in the index and this just exposes that?

phofl · 2021-07-23T18:52:29Z

Could you provide a short example please?

paulg-quantinno · 2021-07-25T21:55:47Z

Sure, here is an example:

data1 = pd.DataFrame({
            'name': ['mary', 'randy'],
            'age': [33, 23],
            'city': ['nyc', 'chi'],
            'date': [pd.to_datetime('20210303'), pd.to_datetime('20210303')],
        })

data2 = pd.DataFrame({
            'name': ['pete', 'mary'],
            'age': [22, 44],
            'city': ['nyc', 'phi'],
            'date': [pd.to_datetime('20210303'), pd.to_datetime('20210505')],
        })

data2= data2.set_index('date')

together = pd.merge_asof(data1.sort_values('date'), data2.sort_values('date'), left_on='date', right_on='date', left_by='city', right_by='city', tolerance=pd.Timedelta('2D'))

`~/anaconda3/envs/dailytestenv/lib/python3.8/site-packages/pandas/core/reshape/merge.py in merge_asof(left, right, on, left_on, right_on, left_index, right_index, by, left_by, right_by, suffixes, tolerance, allow_exact_matches, direction)
579 4 2016-05-25 13:30:00.048 AAPL 98.00 100 NaN NaN
580 """
--> 581 op = _AsOfMerge(
582 left,
583 right,

~/anaconda3/envs/dailytestenv/lib/python3.8/site-packages/pandas/core/reshape/merge.py in init(self, left, right, on, left_on, right_on, left_index, right_index, by, left_by, right_by, axis, suffixes, copy, fill_method, how, tolerance, allow_exact_matches, direction)
1734 self.direction = direction
1735
-> 1736 _OrderedMerge.init(
1737 self,
1738 left,

~/anaconda3/envs/dailytestenv/lib/python3.8/site-packages/pandas/core/reshape/merge.py in init(self, left, right, on, left_on, right_on, left_index, right_index, axis, suffixes, copy, fill_method, how)
1617
1618 self.fill_method = fill_method
-> 1619 _MergeOperation.init(
1620 self,
1621 left,

~/anaconda3/envs/dailytestenv/lib/python3.8/site-packages/pandas/core/reshape/merge.py in init(self, left, right, how, on, left_on, right_on, axis, left_index, right_index, sort, suffixes, copy, indicator, validate)
680 warnings.warn(msg, FutureWarning, stacklevel=3)
681
--> 682 self._validate_specification()
683
684 cross_col = None

~/anaconda3/envs/dailytestenv/lib/python3.8/site-packages/pandas/core/reshape/merge.py in _validate_specification(self)
1782 )
1783 ro_dtype = (
-> 1784 self.right[self.right_on[0]].dtype
1785 if not self.right_index
1786 else self.right.index.dtype

~/anaconda3/envs/dailytestenv/lib/python3.8/site-packages/pandas/core/frame.py in getitem(self, key)
3453 if self.columns.nlevels > 1:
3454 return self._getitem_multilevel(key)
-> 3455 indexer = self.columns.get_loc(key)
3456 if is_integer(indexer):
3457 indexer = [indexer]

~/anaconda3/envs/dailytestenv/lib/python3.8/site-packages/pandas/core/indexes/base.py in get_loc(self, key, method, tolerance)
3361 return self._engine.get_loc(casted_key)
3362 except KeyError as err:
-> 3363 raise KeyError(key) from err
3364
3365 if is_scalar(key) and isna(key) and not self.hasnans:

KeyError: 'date'
`

phofl · 2021-07-25T21:56:58Z

I have fixed this already, will be in 1.3.1

paulg-quantinno · 2021-07-25T21:58:02Z

lol, wow, that was a fast reply, thank you!

phofl added 2 commits May 30, 2020 21:53

Raise ValueError for non numerical join columns in merge_asof

660d2f5

Delete unused imports

cafedcc

dsaxton reviewed May 30, 2020

View reviewed changes

dsaxton added Error Reporting Incorrect or improved errors from pandas Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels May 30, 2020

Implement review requests

fa1d909

Run black pandas

34a5633

phofl changed the title ~~BUG: Raise ValueError for non numerical join columns in merge_asof~~ BUG: Raise TypeError for non numerical join columns in merge_asof May 30, 2020

dsaxton reviewed May 31, 2020

View reviewed changes

pandas/core/reshape/merge.py Outdated Show resolved Hide resolved

Change error message

783bc60

dsaxton approved these changes May 31, 2020

View reviewed changes

doc/source/whatsnew/v1.1.0.rst Outdated Show resolved Hide resolved

Change whats new entry

e086864

jreback requested changes May 31, 2020

View reviewed changes

pandas/core/reshape/merge.py Outdated Show resolved Hide resolved

phofl added 3 commits June 1, 2020 00:02

Merge branch 'master' of https://github.com/pandas-dev/pandas into 29130

a4001c6

Change error type

aaa3480

Move dtype check

ec3147e

phofl changed the title ~~BUG: Raise TypeError for non numerical join columns in merge_asof~~ BUG: Raise ValueError for non numerical join columns in merge_asof May 31, 2020

phofl added 2 commits June 1, 2020 01:17

Use raw string

129eb77

Move check again

4680833

jreback requested changes Jun 1, 2020

View reviewed changes

pandas/core/reshape/merge.py Outdated Show resolved Hide resolved

Move dtype check to _get_merge_keys

fb55c9f

Run black pandas

72fb8b4

jreback requested changes Jun 1, 2020

View reviewed changes

phofl added 2 commits June 1, 2020 18:32

Overwrite _maybe_coerce_merge_keys

0103974

Merge branch 'master' of https://github.com/pandas-dev/pandas into 29130

4cc8bdc

dsaxton mentioned this pull request Sep 16, 2020

CI: Add stale PR action #36336

Merged

dsaxton added the Stale label Sep 16, 2020

github-actions bot added the Stale label Nov 5, 2020

phofl added 2 commits December 29, 2020 22:05

Merge branch 'master' of https://github.com/pandas-dev/pandas into 29130

109c824

� Conflicts: � doc/source/whatsnew/v1.2.0.rst

Move whatsnew

3a66f05

jreback removed the Stale label Dec 29, 2020

jreback requested changes Dec 29, 2020

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into 29130

da377c7

� Conflicts: � doc/source/whatsnew/v1.3.0.rst

jreback requested changes Jan 3, 2021

View reviewed changes

phofl added 4 commits January 4, 2021 14:40

Move merge asof validation

c0ce857

Remove newline

2c164c5

Parametrize test further

a00eb9c

Merge branch 'master' of https://github.com/pandas-dev/pandas into 29130

f62a873

� Conflicts: � doc/source/whatsnew/v1.3.0.rst

jreback reviewed Jan 4, 2021

View reviewed changes

pandas/core/reshape/merge.py Show resolved Hide resolved

Add comment

4782233

jreback requested changes Jan 5, 2021

View reviewed changes

Change Error

6ebb7a3

jreback added this to the 1.3 milestone Jan 6, 2021

jreback approved these changes Jan 6, 2021

View reviewed changes

jreback merged commit cd0224d into pandas-dev:master Jan 6, 2021

phofl deleted the 29130_raise_value_error_non_numerical branch January 6, 2021 00:17

luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021

BUG: Raise ValueError for non numerical join columns in merge_asof (p…

41f400a

…andas-dev#34488)

phofl mentioned this pull request Jul 23, 2021

Regression in merge_asof raising KeyError when at least one by columnis in index #42692

Merged

5 tasks

simonjayhawkins mentioned this pull request Aug 2, 2021

BUG: DatetimeIndex has become unhashable in 1.3.1? #42844

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Raise ValueError for non numerical join columns in merge_asof #34488

BUG: Raise ValueError for non numerical join columns in merge_asof #34488

phofl commented May 30, 2020 •

edited

Loading

dsaxton May 30, 2020

phofl May 30, 2020

dsaxton May 31, 2020

phofl May 31, 2020

pep8speaks commented May 30, 2020 •

edited

Loading

phofl commented Jun 1, 2020

jreback Jun 1, 2020

phofl Jun 1, 2020

dsaxton commented Sep 16, 2020

jreback commented Dec 29, 2020

phofl commented Dec 29, 2020

jreback Dec 29, 2020

phofl Jan 1, 2021

jreback Jan 3, 2021

jreback Jan 3, 2021

jreback Jan 5, 2021

phofl Jan 5, 2021

jreback Jan 5, 2021

phofl Jan 5, 2021

jreback commented Jan 6, 2021

paulg-quantinno commented Jul 23, 2021

phofl commented Jul 23, 2021

paulg-quantinno commented Jul 25, 2021

phofl commented Jul 25, 2021

paulg-quantinno commented Jul 25, 2021

		@@ -1798,6 +1799,24 @@ def _get_merge_keys(self):

		return left_join_keys, right_join_keys, join_names

		def _maybe_coerce_merge_keys(self):

BUG: Raise ValueError for non numerical join columns in merge_asof #34488

BUG: Raise ValueError for non numerical join columns in merge_asof #34488

Conversation

phofl commented May 30, 2020 • 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

pep8speaks commented May 30, 2020 • edited Loading

Comment last updated at 2021-01-05 18:33:30 UTC

phofl commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsaxton commented Sep 16, 2020

jreback commented Dec 29, 2020

phofl commented Dec 29, 2020

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

jreback commented Jan 6, 2021

paulg-quantinno commented Jul 23, 2021

phofl commented Jul 23, 2021

paulg-quantinno commented Jul 25, 2021

phofl commented Jul 25, 2021

paulg-quantinno commented Jul 25, 2021

phofl commented May 30, 2020 •

edited

Loading

pep8speaks commented May 30, 2020 •

edited

Loading