-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
@@ -6,3 +6,6 @@ from numpy cimport int64_t | |||
cdef int64_t cast_from_unit(object ts, object unit) except? -1 | |||
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
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.
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 --diff
Fixed to use same unit parsing logic in
Timedelta
andto_timedelta
. Needs to update after #23264 and #23259.