Skip to content

implement truediv, rtruediv directly in TimedeltaArray; tests #23829

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

Merged
merged 23 commits into from
Nov 29, 2018
Merged

implement truediv, rtruediv directly in TimedeltaArray; tests #23829

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
6ec1f08
implement truediv, rtruediv directly in TimedeltaArray; tests
jbrockmendel Nov 21, 2018
4c2cc59
test tdi/tdi specifically
jbrockmendel Nov 21, 2018
bd2ee96
more checks and test cases
jbrockmendel Nov 21, 2018
3275dd9
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 21, 2018
79901f5
dont define _override_div_mod_methods, matches for pytest.raises
jbrockmendel Nov 21, 2018
adea273
change comment
jbrockmendel Nov 21, 2018
da9f743
whatsnew, GH references
jbrockmendel Nov 21, 2018
ba9e490
error msg py3 compat
jbrockmendel Nov 21, 2018
10bb49b
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 21, 2018
8f276ae
flake8 fixup, raise directly
jbrockmendel Nov 21, 2018
7d56da9
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 24, 2018
6097789
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 26, 2018
2037be8
sidestep object conversion
jbrockmendel Nov 26, 2018
ffedf35
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 27, 2018
2fc44aa
dont case result when operating against object dtype
jbrockmendel Nov 27, 2018
cd4ff57
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 28, 2018
641ad20
another GH reference
jbrockmendel Nov 28, 2018
e0d696f
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 28, 2018
7d9e677
comment
jbrockmendel Nov 28, 2018
dfc7af4
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 28, 2018
55cad6b
Fixup rebase mixup, un-skip part of a test that isnt broken after all
jbrockmendel Nov 29, 2018
d21ae78
Merge branch 'master' of https://github.com/pandas-dev/pandas into gt…
jbrockmendel Nov 29, 2018
d72bf90
flake8 fixup
jbrockmendel Nov 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions pandas/core/arrays/timedeltas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ def _evaluate_with_timedelta_like(self, other, op):
__rfloordiv__ = _wrap_tdi_op(ops.rfloordiv)

def __truediv__(self, other):
# timedelta / X is well-defined for timedelta-like or numeric X
other = lib.item_from_zerodim(other)

if isinstance(other, (ABCSeries, ABCDataFrame, ABCIndexClass)):
Expand Down Expand Up @@ -368,18 +369,14 @@ def __truediv__(self, other):
elif is_object_dtype(other):
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to allow this?
I would be fine with raising a TypeError here.

(I first wanted to say: can't we dispatch that to numpy, thinking that numpy object dtype would handle that, but they raise a TypeError)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can see why this would be first on the chopping block if we had to support fewer cases. Is there a compelling reason not to handle this case?

Copy link
Member

Choose a reason for hiding this comment

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

You can also turn around the question :) Is there a compelling reason to do handle this case?

It's just an extra case to support. And eg, we could discuss whether this should return object dtype data or timedelta, as you are inferring now? Looking at Series behaviour with int64 and object integers, it actually returns object. For datetimes it now raises. So at least, our support is at the moment not very consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a compelling reason to do handle this case?

Because a selling point of pandas is that things Just Work? Because the code and tests are already written, so the marginal cost is \approx zero?

our support is at the moment not very consistent

Fair enough. If a goal is to make things more consistent (which I'm +1 on BTW) then we're probably not going to go around and start breaking the places where it currently is supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree with @jbrockmendel here

result = [self[n] / other[n] for n in range(len(self))]
result = np.array(result)
Copy link
Contributor

Choose a reason for hiding this comment

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

actually this is really close to what soft_convert_objects does.

Copy link
Member Author

Choose a reason for hiding this comment

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

That isn't clear to me. soft_convert_objects doesn't call lib.infer_dtype or any analogue.

Copy link
Contributor

Choose a reason for hiding this comment

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

you are essentially re-implementing it. i would rather not do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not clear on what you have in mind. Something like:

if lib.infer_dtype(result) == 'timedelta':
    result = soft_convert_objects(result, timedelta=True, coerce=False)
    return type(self)(result)
return result

?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, will change

Copy link
Member Author

Choose a reason for hiding this comment

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

changed. this ends up changing the behavior of the DataFrame test case, but that's largely driven by the fact that DataFrame([NaT]) gets inferred as datetime64[ns]

Copy link
Contributor

Choose a reason for hiding this comment

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

what is changed here? shouldn't is_object_type result in a TypeError or a NotImplemented?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two independent questions that have been asked about the object-dtype case:

  1. should we just raise TypeError instead or should we handle it so it Just Works (the latter being what this PR does)
  2. Given that we handle this case, do we try to infer the output dtpye or just return object dtype? This PR originally did the former, then changed to do the latter following discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok this is fine, you are returning object dtype (which is consistent with how we do for Series now)

if lib.infer_dtype(result) == 'timedelta':
# TODO: keep inferred freq?
result, _ = sequence_to_td64ns(result)
return type(self)(result)

return np.array(result)
return result

else:
result = self._data / other
return type(self)(result)

def __rtruediv__(self, other):
# X / timedelta is defined only for timedelta-like X
other = lib.item_from_zerodim(other)

if isinstance(other, (ABCSeries, ABCDataFrame, ABCIndexClass)):
Expand Down
6 changes: 5 additions & 1 deletion pandas/tests/arithmetic/test_timedelta64.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1453,9 +1453,13 @@ def test_td64arr_div_numeric_array(self, box_with_array, vector, dtype):
with pytest.raises(TypeError, match=pattern):
vector / tdser

if not isinstance(vector, pd.Index):
if (not isinstance(vector, pd.Index) and
box_with_array is not pd.DataFrame):
# Index.__rdiv__ won't try to operate elementwise, just raises
# DataFrame casts just-NaT object column to datetime64
result = tdser / vector.astype(object)
expected = [tdser[n] / vector[n] for n in range(len(tdser))]
expected = tm.box_expected(expected, xbox)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this converted the expected list to an DatetimeArray, while the expected result is on object ndarray? (so it's not really testing that?)

Copy link
Member Author

Choose a reason for hiding this comment

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

No. In the case where xbox is tm.to_array (the only case that could conceivably give a DatetimeArray), tm.to_array(any_list) returning np.array(that_list)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK, all a bit opaque .. (I checked what get_upcast_box and box_expected do, but not box_with_array :-))

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah, I'm hoping to simplify some of it, and ideally even get rid of box_expected, but it'll be a while before thats feasible.

tm.assert_equal(result, expected)

with pytest.raises(TypeError, match=pattern):
Expand Down