-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
BUG: Cleanup timedelta offset #23439
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
Conversation
|
Hello @sinhrks! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #23439 +/- ##
==========================================
- Coverage 92.23% 92.23% -0.01%
==========================================
Files 161 161
Lines 51197 51189 -8
==========================================
- Hits 47220 47212 -8
Misses 3977 3977
Continue to review full report at Codecov.
|
pandas/_libs/tslibs/timedeltas.pxd
Outdated
| cpdef int64_t delta_to_nanoseconds(delta) except? -1 | ||
| cpdef convert_to_timedelta64(object ts, object unit) | ||
|
|
||
| # Exposed for Timedelta, not intended for outside use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this cimported by another cython module? If not, doesn’t need to be in the pxd file
d55f00c to
3ade6a9
Compare
jreback
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test comment, otherwise lgtm.
| expected = TimedeltaIndex([np.timedelta64(i, np_unit) | ||
| for i in np.arange(5).tolist()]) | ||
| tm.assert_index_equal(result, expected) | ||
| for wrapper in [np.array, list, pd.Index]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this a parameter (call it box)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, fixed.
3ade6a9 to
04f3ceb
Compare
|
thanks @sinhrks |
|
Personally, I think we should make a distinction between which units are accepted for the For parsing a string, it is fine to accept both '15m', '15min', '15minute', '15minutes and '15T'. But for the (and to be clear, the rest of this PR, fixing the inconsistency of what strings are parsed is of course totally fine!) |
|
@jorisvandenbossche I kept existing behaviour, but it is better to clarify the policy. It should be consistent with period also. |
git diff upstream/master -u -- "*.py" | flake8 --diffFixed to use same unit parsing logic in
Timedeltaandto_timedelta. Needs to update after #23264 and #23259.