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.

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

Fix arithmetic errors with timedelta64 dtypes #22390

Merged
merged 14 commits into from Aug 23, 2018

Conversation

Projects
None yet
2 participants
@jbrockmendel
Copy link
Member

commented Aug 16, 2018

Kind of surprising, but I don't see any Issues for these bugs. Might be because many of them were found after parametrizing arithmetic tests, so instead of Issues we had xfails.

xref #20088
xref #22163

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@codecov

This comment has been minimized.

Copy link

commented Aug 16, 2018

Codecov Report

Merging #22390 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22390      +/-   ##
==========================================
- Coverage   92.04%   92.04%   -0.01%     
==========================================
  Files         169      169              
  Lines       50740    50759      +19     
==========================================
+ Hits        46705    46722      +17     
- Misses       4035     4037       +2
Flag Coverage Δ
#multiple 90.45% <100%> (-0.01%) ⬇️
#single 42.24% <36.84%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/base.py 96.45% <100%> (+0.01%) ⬆️
pandas/core/indexes/range.py 95.73% <100%> (+0.02%) ⬆️
pandas/core/ops.py 96.76% <100%> (+0.04%) ⬆️
pandas/core/arrays/timedeltas.py 91.54% <0%> (-1%) ⬇️

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 25e6a21...cdfb3bf. Read the comment docs.

@@ -576,7 +576,10 @@ Datetimelike
- Bug in :class:`DataFrame` with mixed dtypes including ``datetime64[ns]`` incorrectly raising ``TypeError`` on equality comparisons (:issue:`13128`,:issue:`22163`)
- Bug in :meth:`DataFrame.eq` comparison against ``NaT`` incorrectly returning ``True`` or ``NaN`` (:issue:`15697`,:issue:`22163`)
- Bug in :class:`DataFrame` with ``timedelta64[ns]`` dtype division by ``Timedelta``-like scalar incorrectly returning ``timedelta64[ns]`` dtype instead of ``float64`` dtype (:issue:`20088`,:issue:`22163`)

This comment has been minimized.

Copy link
@jreback

jreback Aug 17, 2018

Contributor

can you split out to a timedelta bug section (as this is getting kind of long)?

can be in a future PR

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Aug 17, 2018

Author Member

Sounds good

This comment has been minimized.

Copy link
@jreback

jreback Aug 22, 2018

Contributor

I think we now have a separate section?

@@ -596,6 +597,9 @@ def _evaluate_numeric_binop(self, other):
# GH#19333 is_integer evaluated True on timedelta64,
# so we need to catch these explicitly
return op(self._int64index, other)
elif is_timedelta64_dtype(other):

This comment has been minimized.

Copy link
@jreback

jreback Aug 17, 2018

Contributor

doesn't the fall thru raise TypeError?

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Aug 18, 2018

Author Member

I don’t understand the question

This comment has been minimized.

Copy link
@jreback

jreback Aug 20, 2018

Contributor

your comment is at odds with the checking i think. pls revise comments and/or checks.

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Aug 20, 2018

Author Member

The comment below this line # must be an np.ndarray; GH#22390 along with the check above elif is_timedelta64_dtype(other) is just saying this is an ndarray["timedelta64['ns']"]. I don't think these are at odds.

# timedelta64 dtypes because numpy casts integer dtypes to
# timedelta64 when operating with timedelta64
if isinstance(right, np.ndarray):
# upcast to TimedeltaIndex before dispatching

This comment has been minimized.

Copy link
@jreback

jreback Aug 17, 2018

Contributor

uhh this seems like a lot of special casing, any way to make this simpler

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Aug 17, 2018

Author Member

The only simplification I considered but didn't use was something like:

def maybe_upcast_for_op(obj):
     if type(obj) is timedelta:
        return pd.Timedelta(obj)
    if isinstance(obj, np.ndarray) and is_timedelta64_dtype(obj):
        return pd.TimedeltaIndex(obj)
    return obj

which would go right after get_op_result_name.

This comment has been minimized.

Copy link
@jreback

jreback Aug 20, 2018

Contributor

maybe re-route the scalars to a separate path. This is getting way special casey here.

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Aug 20, 2018

Author Member

I'll implement the maybe_upcast idea and see if that is prettier

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

This will probably need rebasing after #22350 goes through

index=left.index, name=res_name,
dtype=result.dtype)

elif type(right) is datetime.timedelta:

This comment has been minimized.

Copy link
@jreback

jreback Aug 20, 2018

Contributor

this case you can include above (before what you added), no?

if isintance(right, (datetime.timedelta, Timedelta)):
  right = Timedelta(right)
....

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Aug 20, 2018

Author Member

The version code above casts to TimedeltaIndex, not Timedelta.

# timedelta64 dtypes because numpy casts integer dtypes to
# timedelta64 when operating with timedelta64
if isinstance(right, np.ndarray):
# upcast to TimedeltaIndex before dispatching

This comment has been minimized.

Copy link
@jreback

jreback Aug 20, 2018

Contributor

maybe re-route the scalars to a separate path. This is getting way special casey here.

@@ -596,6 +597,9 @@ def _evaluate_numeric_binop(self, other):
# GH#19333 is_integer evaluated True on timedelta64,
# so we need to catch these explicitly
return op(self._int64index, other)
elif is_timedelta64_dtype(other):

This comment has been minimized.

Copy link
@jreback

jreback Aug 20, 2018

Contributor

your comment is at odds with the checking i think. pls revise comments and/or checks.

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

Implemented maybe_upcast_for_op. There may be other places where this can be used; I'll take a look in a follow-up.

@jreback
Copy link
Contributor

left a comment

minor doc-comment. rebase and ping on green.

@@ -576,7 +576,10 @@ Datetimelike
- Bug in :class:`DataFrame` with mixed dtypes including ``datetime64[ns]`` incorrectly raising ``TypeError`` on equality comparisons (:issue:`13128`,:issue:`22163`)
- Bug in :meth:`DataFrame.eq` comparison against ``NaT`` incorrectly returning ``True`` or ``NaN`` (:issue:`15697`,:issue:`22163`)
- Bug in :class:`DataFrame` with ``timedelta64[ns]`` dtype division by ``Timedelta``-like scalar incorrectly returning ``timedelta64[ns]`` dtype instead of ``float64`` dtype (:issue:`20088`,:issue:`22163`)

This comment has been minimized.

Copy link
@jreback

jreback Aug 22, 2018

Contributor

I think we now have a separate section?

@jreback jreback added this to the 0.24.0 milestone Aug 22, 2018

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

ping

@jreback jreback merged commit dd24e76 into pandas-dev:master Aug 23, 2018

6 checks passed

ci/circleci: py27_compat Your tests passed on CircleCI!
Details
ci/circleci: py35_ascii Your tests passed on CircleCI!
Details
ci/circleci: py36_locale Your tests passed on CircleCI!
Details
ci/circleci: py36_locale_slow Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

thanks! nice cleaning

@jbrockmendel jbrockmendel deleted the jbrockmendel:rbug branch Aug 23, 2018

Sup3rGeo added a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.