Skip to content
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.

Sign up for GitHub

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

Jump to bottom

issue #27642 - timedelta merge asof with tolerance #27650

Merged
merged 25 commits into from
Aug 22, 2019
Merged

issue #27642 - timedelta merge asof with tolerance #27650

Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
533d407
issue #27642 - timedelta merge asof with tolerance
ianzur Jul 29, 2019
431da66
#27642
ianzur Jul 29, 2019
c9acc09
whatsnew
ianzur Jul 30, 2019
76f6525
#27642 added testing stuff
ianzur Jul 30, 2019
fd1dbb2
mroeschke: this test is for you :rocket:
ianzur Jul 30, 2019
63b7a2b
some words
ianzur Jul 30, 2019
84759ac
remove some whitespace
ianzur Jul 30, 2019
04c18e9
a comment
ianzur Jul 30, 2019
ac76918
docs change
ianzur Jul 31, 2019
0a0c5c8
Merge remote-tracking branch 'upstream/master' into issue-#27642-time…
ianzur Jul 31, 2019
a26bb71
callbacks are :fireworks:
ianzur Jul 31, 2019
a0ba5d9
whitespace
ianzur Jul 31, 2019
e5456ac
little stuff
ianzur Jul 31, 2019
4f56dc4
isort
ianzur Aug 2, 2019
52193a4
I think this'll work
ianzur Aug 2, 2019
7c004c9
cleaner
ianzur Aug 2, 2019
3f8bf2b
Merge remote-tracking branch 'upstream/master' into issue-#27642-time…
ianzur Aug 8, 2019
8f8aeb3
:bug: :wrench:
ianzur Aug 9, 2019
f66fce4
blacked
ianzur Aug 9, 2019
0eb173f
unused import
ianzur Aug 9, 2019
4a1c515
Merge remote-tracking branch 'upstream/master' into issue-#27642-time…
ianzur Aug 9, 2019
6daf823
resolved merge conflict in whatsnew
ianzur Aug 16, 2019
3688986
Merge remote-tracking branch 'upstream/master' into ianzur-issue-#276…
TomAugspurger Aug 20, 2019
f6009ca
Merge remote-tracking branch 'upstream/master' into ianzur-issue-#276…
TomAugspurger Aug 22, 2019
6f7151d
Removed duplicate
TomAugspurger Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion pandas/core/reshape/merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
is_number,
is_numeric_dtype,
is_object_dtype,
is_timedelta64_dtype,
needs_i8_conversion,
)
from pandas.core.dtypes.missing import isnull, na_value_for_dtype
Expand Down Expand Up @@ -1635,7 +1636,11 @@ def _get_merge_keys(self):
)
)

if is_datetime64_dtype(lt) or is_datetime64tz_dtype(lt):
if(
ianzur marked this conversation as resolved.
Show resolved Hide resolved
is_datetime64_dtype(lt)
or is_datetime64tz_dtype(lt)
or is_timedelta64_dtype(lt)
):
if not isinstance(self.tolerance, Timedelta):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be if not isinstance(self.tolerance, datetime.timedelta) so users can pass datetime.timedelta objects. If you could add a test case for that situation that'd be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see commit "mroeschke: this test is for you 🚀"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test fails

Copy link
Contributor Author

@ianzur ianzur Jul 30, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need a different PR for this test I think, it fails at this conversion check.

pandas/pandas/core/reshape/merge.py

Lines 1704 to 1710 in 143bc34

# initial type conversion as needed
if needs_i8_conversion(left_values):
left_values = left_values.view("i8")
right_values = right_values.view("i8")
if tolerance is not None:
tolerance = tolerance.value

With tolerance = datetime.timedelta -> ERROR: has no attribute "value"

Copy link
Contributor Author

@ianzur ianzur Jul 30, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datetime.timedelta objects do not have nanoseconds like pd.Timedelta

Another check and conversion is necessary to convert datetime.timedelta correctly to nanoseconds similarly to pd.Timedelta.value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks for investigating. Yes, I'd be great to address in another issues/PR.

raise MergeError(msg)
if self.tolerance < Timedelta(0):
Expand Down