Skip to content
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

Improve DatetimeIndex.time performance #18461

Merged
merged 3 commits into from Dec 10, 2017

Conversation

Projects
None yet
3 participants
@tmnhat2001
Copy link
Contributor

tmnhat2001 commented Nov 24, 2017

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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.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.

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")

This comment has been minimized.

Copy link
@bashtage

bashtage Nov 24, 2017

Contributor

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

@@ -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

This comment has been minimized.

Copy link
@jreback

jreback Nov 24, 2017

Contributor

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

@jreback

This comment has been minimized.

Copy link
Contributor

jreback commented Nov 24, 2017

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

@tmnhat2001

This comment has been minimized.

Copy link
Contributor Author

tmnhat2001 commented Dec 9, 2017

@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

This comment has been minimized.

Copy link
Contributor

jreback commented Dec 9, 2017

can u rebase

@tmnhat2001 tmnhat2001 force-pushed the tmnhat2001:dti-time-perf branch from c274e8d to a132c6d Dec 9, 2017

@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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Copy link
Contributor

jreback commented Dec 10, 2017

thanks @tmnhat2001

@tmnhat2001

This comment has been minimized.

Copy link
Contributor Author

tmnhat2001 commented Dec 10, 2017

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.