-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Trim unncessary code in datetime/np_datetime.c #21962
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
@@ -147,6 +147,9 @@ cdef inline void td64_to_tdstruct(int64_t td64, | |||
|
|||
cdef inline int64_t pydatetime_to_dt64(datetime val, | |||
npy_datetimestruct *dts): | |||
""" |
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.
add an explicit check?
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.
This is called by some very perf-sensitive code, all uses of which already handle this carefully. I think its sufficiently internal to be OK without.
Codecov Report
@@ Coverage Diff @@
## master #21962 +/- ##
=======================================
Coverage 91.96% 91.96%
=======================================
Files 166 166
Lines 50329 50329
=======================================
Hits 46287 46287
Misses 4042 4042
Continue to review full report at Codecov.
|
thanks @jbrockmendel |
pydatetime_to_datetimestruct
does a ton of checking that boils down to "is this a valid datetime object?" Since the function only gets called after a type-check, we can assume it is a date/datetime and be a lot less verbose about it.This also rips out an unnecessary layer of functions
convert_datetime_to_datetimestruct
,convert_timedelta_to_timedeltastruct
.cc @WillAyd you mentioned wanting to work on your C-foo. There's a comment about figuring out how to import the cpython datetime C-API. Any thoughts?