Skip to content

Bug: merge_asof() cannot use datetime.timedelta as tolerance kwarg #28100

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

Closed
wants to merge 1 commit into from
Closed

Bug: merge_asof() cannot use datetime.timedelta as tolerance kwarg #28100

wants to merge 1 commit into from

Conversation

ianzur
Copy link
Contributor

@ianzur ianzur commented Aug 22, 2019

@ianzur
Copy link
Contributor Author

ianzur commented Aug 22, 2019

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
@TomAugspurger

@ianzur ianzur changed the title Bug #28098: merge_asof() cannot use datetime.timedelta as tolerance kwarg Bug: merge_asof() cannot use datetime.timedelta as tolerance kwarg Aug 22, 2019
@ianzur
Copy link
Contributor Author

ianzur commented Aug 22, 2019

The offending test

@pytest.mark.parametrize(
"tolerance",
[
Timedelta("1day"),
pytest.param(
datetime.timedelta(days=1),
marks=pytest.mark.xfail(reason="not implemented", strict=True),
),
],
ids=["pd.Timedelta", "datetime.timedelta"],
)
def test_tolerance(self, tolerance):
trades = self.trades
quotes = self.quotes
result = merge_asof(trades, quotes, on="time", by="ticker", tolerance=tolerance)
expected = self.tolerance
assert_frame_equal(result, expected)

@WillAyd
Copy link
Member

WillAyd commented Aug 25, 2019

I only see a whatsnew - did you have an actual fix and tests for this?

@WillAyd WillAyd added Datetime Datetime data dtype Timedelta Timedelta data type and removed Datetime Datetime data dtype labels Aug 25, 2019
@ianzur
Copy link
Contributor Author

ianzur commented Aug 27, 2019

@WillAyd

See this conversation. I found this bug while writing a fix for a separate issue.

https://github.com/pandas-dev/pandas/pull/27650/files/04c18e9f15358ac2b91631abfaf0f4151cf289cf#r308457291

I was asked to add this test in a separate pull request.
You are right I have not written a fix, simply investigated the bug.

@WillAyd
Copy link
Member

WillAyd commented Aug 27, 2019

Oh OK thanks. Let's close this for now though until a fix is in place. Usually prefer to keep our PR queue down to actionable items so can come back to this when there is something to review

@WillAyd WillAyd closed this Aug 27, 2019
@ianzur
Copy link
Contributor Author

ianzur commented Aug 28, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants