-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
PERF: fixed performance degration becasue doesn't slice values array in to_native_types of DateTimeBlock #25765
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
PERF: fixed performance degration becasue doesn't slice values array in to_native_types of DateTimeBlock #25765
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25765 +/- ##
==========================================
+ Coverage 91.25% 91.25% +<.01%
==========================================
Files 172 172
Lines 52977 52978 +1
==========================================
+ Hits 48342 48344 +2
+ Misses 4635 4634 -1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25765 +/- ##
==========================================
- Coverage 91.99% 91.98% -0.01%
==========================================
Files 175 175
Lines 52382 52383 +1
==========================================
- Hits 48188 48184 -4
- Misses 4194 4199 +5
Continue to review full report at Codecov.
|
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.
tests pls
or if this is a perf fix, then show the updated asv's (and/or add one) |
Hello @hksonngan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-04-19 00:10:51 UTC |
@jreback I add asv's test, test with my macair 2012.
|
@jreback ping |
@hksonngan I think you ran asv dev, you need to run asv continuous here it should the ratio of the change. |
also pls add a whatsnew note. in performance improvements. |
@jreback I think for small under 10.000obs is not gain to notice
|
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.
also i am not clear you are actually fixing anything. pls run your example with timeit on master and with the PR.
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.
pls show a timeit before/after this PR
can you merge master and update |
@jreback Here is CProfile upstream/master and branch fix. |
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.
pls also show the asv's results
lgtm. ping on green. |
@jreback ping. |
''' [ 75.00%] · For pandas commit 7b3bf2d (round 2/2):
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
thanks @hksonngan |
Can someone check the perf for this benchmark: http://pandas.pydata.org/speed/pandas/index.html#series_methods.IsInForObjects.time_isin_long_series_long_values_floats?commits=f53bb061939fb54d6c826c4db408c458d3d2a1db Not sure if real or a spurious regression. |
@TomAugspurger this is real, because of setting this params = [1000, 10000, 100000]. |
I don’t think so. The regression is for a different benchmark, no? |
git diff upstream/master -u -- "*.py" | flake8 --diff
Slicing in line 2181 only slice of i8values that make a whole of values array is processed in _is_dates_only function. This change was from pull diff reduction for 24024 #24543