implement TimedeltaArray asm8, to_timedelta64 #23205
Conversation
|
Hello @jbrockmendel! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #23205 +/- ##
=========================================
Coverage ? 92.19%
=========================================
Files ? 169
Lines ? 50958
Branches ? 0
=========================================
Hits ? 46979
Misses ? 3979
Partials ? 0
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Just a general comment: for Timestamp / DatetimeIndex you have the same inconsistency of to_datetime64.
Also on the Index/Array classes, you already have other ways to get the numpy array (np.array(..), although might sometimes give object dtype). Mainly wondering if we should have a more general API across the arrays to convert to certain numpy types.
| -------- | ||
| Timedelta.to_timedelta64 | ||
| """ | ||
| return self.asi8.view('m8[ns]') |
There was a problem hiding this comment.
you are going a little bit in circles here, as asi8 itself is already self.values.view('i8') ?
There was a problem hiding this comment.
I find asi8 much more explicit than alues (BTW as I mentioned to jreback the "v" key on my keyboard is sticky; I'm going to stop copy-pasting and let you guys infer the missing letter for a while).
| return self.asi8.view('m8[ns]') | ||
|
|
||
| @property | ||
| def asm8(self): |
There was a problem hiding this comment.
do we need both to_timedelta64 and asm8 ?
There was a problem hiding this comment.
If we were starting fresh I would say no, but as it is I think consistency with the scalar type is really worthwhile. (and to your point aboe DatetimeIndex should have a to_datetime64 method)
|
Updated to implement DatetimeArray.asm8, DatetimeArray.to_datetime64 |
|
Thinking about this a bit more: I am -1 on adding duplicate attributes/methods just for sake of consistency with our scalar type Also for DatetimeIndex/TimedeltaIndex, it is not really clear to me what this is targeted use case is, as it just returns a copy of itself? (except that it returns an array instead of index? Which I also don't find clear from the name) |
Would you advocate removing one/both of the attributes from the Timedelta/Timestamp scalars? |
|
lgtm. waiting for CI to merge. |
|
thanks! |
This reverts commit 6e50713.
…)" This reverts commit 6e50713.
|
Sorry, Jeff, but I clearly objected this PR. Can you then at least discuss it before merging? |
|
@jorisvandenbossche if u object then your need to actually put in a review request to do so |
…)" (pandas-dev#23290) This reverts commit 6e50713.
For compat with Timedelta scalar
git diff upstream/master -u -- "*.py" | flake8 --diff