ASV/CLN: cleanup period benchmarks #18275

jbrockmendel · 2017-11-14T02:27:33Z

Fix one benchmark that wasnt getting run because of a name typo

Remove redundant Period benchmarks.

Cleanup a few benchmarks using params.

#noci
(is that how that works?)

codecov · 2017-11-14T07:30:40Z

Codecov Report

Merging #18275 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18275      +/-   ##
==========================================
- Coverage    91.4%   91.36%   -0.05%     
==========================================
  Files         164      164              
  Lines       49878    49878              
==========================================
- Hits        45590    45569      -21     
- Misses       4288     4309      +21

Flag	Coverage Δ
#multiple	`89.16% <ø> (-0.03%)`	⬇️
#single	`39.41% <ø> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
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 69472f9...b6551f7. Read the comment docs.

codecov · 2017-11-14T07:30:55Z

Codecov Report

Merging #18275 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18275      +/-   ##
==========================================
- Coverage   91.35%   91.34%   -0.02%     
==========================================
  Files         163      163              
  Lines       49714    49691      -23     
==========================================
- Hits        45415    45388      -27     
- Misses       4299     4303       +4

Flag	Coverage Δ
#multiple	`89.14% <ø> (ø)`	⬆️
#single	`39.67% <ø> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️
pandas/io/sql.py	`94.79% <0%> (-0.01%)`	⬇️
pandas/core/reshape/reshape.py	`100% <0%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`95.49% <0%> (ø)`	⬆️
pandas/io/formats/format.py	`96.01% <0%> (ø)`	⬆️
pandas/core/indexes/multi.py	`96.41% <0%> (ø)`	⬆️
pandas/core/dtypes/missing.py	`90.74% <0%> (+0.11%)`	⬆️
pandas/tseries/offsets.py	`97% <0%> (+0.3%)`	⬆️

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 103ea6f...27d75c3. Read the comment docs.

jorisvandenbossche · 2017-11-14T12:26:23Z

#noci (is that how that works?)

No, but you can add [ci skip] in the commit message (https://docs.travis-ci.com/user/customizing-the-build/#Skipping-a-build)

jorisvandenbossche · 2017-11-14T12:27:01Z

asv_bench/benchmarks/period.py

-
-    def time_quarter(self):
-        self.per.quarter
+    params = ['M', 'min']


does this have to be double brackets ?

Not in my admittedly-limited experience. Though the only examples that use both params and param_names do have 2+ param_names, so do have double double-brackets for params.

jorisvandenbossche · 2017-11-14T12:29:12Z

asv_bench/benchmarks/period.py

+        df['col'] = rng
+
+
+class PeriodIndexAlgorithms(object):


you could also param this one (I would just call it Algorithms, the 'period' is already in the file name) for ['series', 'index']

I thought about that but decided against it because the two cases treat the data differently, would need a if cls is Series: ... in the setup method. But heck, let's go for it.

I would rather do a data = ...; if typ == 'series': data = pd.Series(data)

jorisvandenbossche · 2017-11-14T12:29:32Z

asv_bench/benchmarks/period.py

+        self.series.value_counts()
+
+
+class PeriodStandardIndexing(object):


I would just do 'Indexing' ?

Name used to be period_standard_indexing

Yes, and the one in your PR is certainly an improvement (since it follows PEP8), but I think the 'period' and 'standard' are just redundant (unless there is another class with non-standard indexing)

jorisvandenbossche · 2017-11-14T12:30:34Z

asv_bench/benchmarks/period.py

@@ -101,7 +28,7 @@ def time_second(self):
        self.per.second

    def time_is_leap_year(self):
-        self.per.is_leap_year
+        self.per.is_leapyear


I think is_leap_year was actually correct ?

Yep, you're right. One of the two existing benchmarks used each and I guessed wrong.

jorisvandenbossche · 2017-11-14T16:09:19Z

asv_bench/benchmarks/period.py

+class Algorithms(object):
+    goal_time = 0.2
+
+    params = [PeriodIndex, Series]


what does this give in the asv run output as name for the benchmark? (just 'PeriodIndex' or the full path? in the last case I would just do ['index', 'series'])

what does this give in the asv run output as name for the benchmark?

I'm not sure. Unfortunately I'm not aware of a quick-feedback way of running these.

Going to put a pin in this PR for a bit, much rather focus on offsets where current PRs need to get resolved before I can do actual bugfixes.

Check 'asv dev', you can select only this file and specify a quick run

Quickly checked this (with asv dev -b period and it used the full path, so please use ['index', 'series'] as the params

jreback · 2017-11-15T11:32:51Z

lgtm. ping when you have updated the comments to @jorisvandenbossche quesiton.

jorisvandenbossche · 2017-11-15T16:07:53Z

asv_bench/benchmarks/period.py

+        self.rng = date_range('1985', periods=1000)
+        self.rng2 = date_range('1985', periods=1000).to_pydatetime()
+
+    def time_from_date_range(self):


you also need to pass the param in the actual time_ functions as well. So time_from_date_range(self, freq) in this case (and also for all other benchmarks)

Is this in addition to or instead of setting self.freq=freq in setup?

that doesn't matter, it is just needed to let the code actually run.
But since you have to do that, the self.freq = freq is a bit superfluous as you can also do PeriodIndex(.. freq=freq). So to conclude: instead

jreback · 2017-11-22T02:28:51Z

can you update

…v_cleanup

jorisvandenbossche

Please run the benchmarks (eg asv dev -b period does not take a long time) to make sure they at least work.

jorisvandenbossche · 2017-11-22T21:59:06Z

asv_bench/benchmarks/period.py

-    def setup(self):
-        self.per = Period('2017-09-06 08:28', freq='min')
+    def time_now(self, freq):
+        self.per.now()


This one is failing (needs a freq)

jorisvandenbossche · 2017-11-22T22:01:18Z

asv_bench/benchmarks/period.py

-    def time_to_timestamp():
-        self.per.to_timestamp()
+        if typ == 'index':
+            self.vector = PeriodIndex(data, freq='M')


I would do here data * 1000 as well like you did for series (to have more data)

Happy to make that change, but the *1000 for series was there when I got here. Im implicitly assuming someone had a reason for doing it that way.

I don't think there was a reason

jbrockmendel added 2 commits November 13, 2017 18:04

Cleanup/parametrize asv

010e80d

Fix typo

b6551f7

jorisvandenbossche added the Benchmark Performance (ASV) benchmarks label Nov 14, 2017

jorisvandenbossche reviewed Nov 14, 2017

View reviewed changes

reviewer comments, separate UnaryMethods, flake8 fixup

41ec6e0

jorisvandenbossche reviewed Nov 14, 2017

View reviewed changes

jreback added this to the 0.22.0 milestone Nov 15, 2017

jorisvandenbossche reviewed Nov 15, 2017

View reviewed changes

jbrockmendel added 2 commits November 21, 2017 19:29

Merge branch 'master' of https://github.com/pandas-dev/pandas into as…

4c8e8e2

…v_cleanup

pass params to all methods

e19d397

jorisvandenbossche reviewed Nov 22, 2017

View reviewed changes

fix missing freq arg

27d75c3

jorisvandenbossche changed the title ~~ASV cleanup~~ ASV/CLN: cleanup period benchmarks Nov 23, 2017

jorisvandenbossche merged commit c634352 into pandas-dev:master Nov 23, 2017

jbrockmendel deleted the asv_cleanup branch December 8, 2017 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASV/CLN: cleanup period benchmarks #18275

ASV/CLN: cleanup period benchmarks #18275

jbrockmendel commented Nov 14, 2017

codecov bot commented Nov 14, 2017

codecov bot commented Nov 14, 2017 •

edited

Loading

jorisvandenbossche commented Nov 14, 2017

jorisvandenbossche Nov 14, 2017

jbrockmendel Nov 14, 2017

jorisvandenbossche Nov 14, 2017

jbrockmendel Nov 14, 2017

jorisvandenbossche Nov 14, 2017

jorisvandenbossche Nov 14, 2017

jbrockmendel Nov 14, 2017

jorisvandenbossche Nov 14, 2017

jorisvandenbossche Nov 14, 2017

jbrockmendel Nov 14, 2017

jorisvandenbossche Nov 14, 2017

jbrockmendel Nov 14, 2017

jorisvandenbossche Nov 14, 2017

jorisvandenbossche Nov 15, 2017

jreback commented Nov 15, 2017

jorisvandenbossche Nov 15, 2017

jbrockmendel Nov 22, 2017

jorisvandenbossche Nov 22, 2017

jreback commented Nov 22, 2017

jorisvandenbossche left a comment

jorisvandenbossche Nov 22, 2017

jorisvandenbossche Nov 22, 2017

jbrockmendel Nov 22, 2017

jorisvandenbossche Nov 22, 2017

		self.series.value_counts()


		class PeriodStandardIndexing(object):

ASV/CLN: cleanup period benchmarks #18275

ASV/CLN: cleanup period benchmarks #18275

Conversation

jbrockmendel commented Nov 14, 2017

codecov bot commented Nov 14, 2017

Codecov Report

codecov bot commented Nov 14, 2017 • edited Loading

Codecov Report

jorisvandenbossche commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 22, 2017

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2017 •

edited

Loading