Skip to content

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

Merged
merged 3 commits into from
Jul 20, 2018

Conversation

jbrockmendel
Copy link
Member

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?

@gfyoung gfyoung added Datetime Datetime data dtype Internals Related to non-user accessible pandas implementation Clean labels Jul 18, 2018
@@ -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):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

add an explicit check?

Copy link
Member Author

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
Copy link

codecov bot commented Jul 18, 2018

Codecov Report

Merging #21962 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #21962   +/-   ##
=======================================
  Coverage   91.96%   91.96%           
=======================================
  Files         166      166           
  Lines       50329    50329           
=======================================
  Hits        46287    46287           
  Misses       4042     4042
Flag Coverage Δ
#multiple 90.36% <ø> (ø) ⬆️
#single 42.23% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 96.46% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b73f22...69a8459. Read the comment docs.

@jreback jreback added this to the 0.24.0 milestone Jul 20, 2018
@jreback jreback merged commit 1a586e2 into pandas-dev:master Jul 20, 2018
@jreback
Copy link
Contributor

jreback commented Jul 20, 2018

thanks @jbrockmendel

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
@jbrockmendel jbrockmendel deleted the cless branch April 5, 2020 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Datetime Datetime data dtype Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants