Skip to content

Improve DatetimeIndex.time performance #18461

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 10, 2017
Merged

Improve DatetimeIndex.time performance #18461

merged 3 commits into from
Dec 10, 2017

Conversation

jamestran201
Copy link

@jamestran201 jamestran201 commented Nov 24, 2017 •
edited
Loading

xref #18058

  • tests added / passed

  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

  • whatsnew entry

    Speed up DatetimeIndex.time in a similar way to DatetimeIndex.date. Not sure whether the timezone information should be passed to ints_to_pydatetime() or not.

@codecov
Copy link

codecov bot commented Nov 24, 2017

Codecov Report

Merging #18461 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18461      +/-   ##
==========================================
- Coverage   91.35%   91.31%   -0.05%     
==========================================
  Files         163      163              
  Lines       49695    49695              
==========================================
- Hits        45401    45380      -21     
- Misses       4294     4315      +21
Flag Coverage Δ
#multiple 89.11% <100%> (-0.03%) ⬇️
#single 39.66% <0%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/datetimes.py 95.49% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 63.44% <0%> (-1.82%) ⬇️
pandas/core/frame.py 97.8% <0%> (-0.1%) ⬇️

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 e6eac0b...c274e8d. Read the comment docs.

@codecov
Copy link

codecov bot commented Nov 24, 2017

Codecov Report

Merging #18461 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18461      +/-   ##
==========================================
- Coverage   91.35%   91.31%   -0.05%     
==========================================
  Files         163      163              
  Lines       49695    49695              
==========================================
- Hits        45401    45380      -21     
- Misses       4294     4315      +21
Flag Coverage Δ
#multiple 89.11% <100%> (-0.03%) ⬇️
#single 39.66% <0%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/datetimes.py 95.49% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 63.44% <0%> (-1.82%) ⬇️
pandas/core/frame.py 97.8% <0%> (-0.1%) ⬇️

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 e6eac0b...c274e8d. Read the comment docs.

@codecov
Copy link

codecov bot commented Nov 24, 2017 •
edited
Loading

Codecov Report

Merging #18461 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18461      +/-   ##
==========================================
- Coverage    91.6%   91.56%   -0.05%     
==========================================
  Files         153      153              
  Lines       51273    51273              
==========================================
- Hits        46970    46947      -23     
- Misses       4303     4326      +23
Flag Coverage Δ
#multiple 89.42% <100%> (-0.03%) ⬇️
#single 40.68% <0%> (-0.11%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/datetimes.py 95.68% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 64.78% <0%> (-1.74%) ⬇️
pandas/util/testing.py 81.82% <0%> (-0.2%) ⬇️
pandas/core/frame.py 97.81% <0%> (-0.1%) ⬇️

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 34a8d36...9734ae2. Read the comment docs.

bashtage
bashtage reviewed Nov 24, 2017
View reviewed changes
pandas/_libs/tslib.pyx Outdated
If Timestamp, convert to pandas.Timestamp

Returns
-------
result : array of dtype specified by box
"""

assert ((box == "datetime") or (box == "date") or (box == "timestamp")), \
"box must be one of 'datetime', 'date' or 'timestamp'"
assert ((box == "datetime") or (box == "date") or (box == "timestamp")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use box in ('datetime', 'date',...)? Seems unnecessarily C-like

jreback
jreback requested changes Nov 24, 2017
View reviewed changes
pandas/_libs/tslib.pyx
@@ -125,6 +133,8 @@ def ints_to_pydatetime(ndarray[int64_t] arr, tz=None, freq=None,
if is_string_object(freq):
from pandas.tseries.frequencies import to_offset
freq = to_offset(freq)
elif box == "time":
func_create = create_time_from_ts
elif box == "datetime":
func_create = create_datetime_from_ts
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, can remove the assert and make else the raise case

@jreback jreback added Performance Memory or execution speed performance Datetime Datetime data dtype labels Nov 24, 2017
@jreback
Copy link
Contributor

jreback commented Nov 24, 2017

can you post the result of the asv as well (IOW how well does this do)

@jamestran201
Copy link
Author

@jreback @bashtage sorry for the late reply. The asv is:

             before                 after     ratio
         [e6eac0b3]            [c274e8d5]
-        9.17±0.2ms           1.17±0.02ms     0.13   timeseries.DatetimeIndex.time_dti_time

should any other changes be made in addition to changing the assertion to an else block?

@jreback
Copy link
Contributor

jreback commented Dec 9, 2017

can u rebase

@jreback jreback added this to the 0.22.0 milestone Dec 10, 2017
@jreback jreback merged commit 451811d into pandas-dev:master Dec 10, 2017
@jreback
Copy link
Contributor

jreback commented Dec 10, 2017

thanks @tmnhat2001

@jamestran201
Copy link
Author

thanks

@jamestran201 jamestran201 deleted the dti-time-perf branch December 10, 2017 19:49
This was referenced May 31, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bashtage bashtage bashtage left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Datetime Datetime data dtype Performance Memory or execution speed performance
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jamestran201 @jreback @bashtage @jamestran201-alt