classification
Title: timeit: Use thousands separators and print number of loops per second
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: cheryl.sabella, ezio.melotti, jstasiak, pitrou, r.david.murray, serhiy.storchaka, tim.peters
Priority: normal Keywords: patch

Created on 2013-09-08 16:00 by jstasiak, last changed 2017-08-27 13:46 by cheryl.sabella.

Files
File name Uploaded Description Edit
timeit.patch jstasiak, 2013-09-08 16:00 review
timeit-v2.patch jstasiak, 2013-09-08 16:16 review
timeit-v3-actual-changes.patch jstasiak, 2013-09-08 19:48 review
timeit-v3-pep8.patch jstasiak, 2013-09-08 19:48
timeit-v3-actual-changes-no-separators.patch jstasiak, 2013-09-16 01:17 review
timeit-v4-actual-changes.patch jstasiak, 2013-09-16 09:54 review
Messages (18)
msg197274 - (view) Author: Jakub Stasiak (jstasiak) * Date: 2013-09-08 16:00
This patch includes:
* making code more PEP8-compatible and refactoring it a bit
* printing number of loops per second when using command line interface
* using thousands separators when printing numbers of loops (also in command line interface)
* changing examples in the module documentation

The output is changed from this:

    10000 loops, best of 3: 40.3 usec per loop

to that:

    10,000 loops, best of 3: 34.6 usec per loop, 28,870.783/s

I'm still not sure about details of "28,870.783/s" part:
* whether it should always include the fractional part (in this example it doesn't make any sense)
* maybe it should say "loops/s" rather than just "/s"
msg197276 - (view) Author: Jakub Stasiak (jstasiak) * Date: 2013-09-08 16:16
Oops, forgot to patch the tests, please find correct patch attached.
msg197282 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-09-08 16:41
I personally dislike the "," as thousands separator, and if a separator is added at all I would prefer a space as defined in the SI standard[0].

The PEP8 changes should also me moved to a separate patch IMHO; the other changes are OK grouped together.

[0]: http://en.wikipedia.org/wiki/ISO_31-0#Numbers
msg197286 - (view) Author: Jakub Stasiak (jstasiak) * Date: 2013-09-08 16:53
I agree with both notes. Splitting the patch won't be a problem.

As much as I don't fancy "," as thousands separator either - I just used what's in the standard library but I'll think about the best way of putting spaces there.
msg197287 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-08 17:03
> I personally dislike the "," as thousands separator, and if a
> separator is added at all I would prefer a space as defined in the SI 
> standard[0].

Then you really want a non-breaking space ;-)

(as a French person who's used to commas as decimal points, count me in the "commas as thousands separators are confusing" camp ;-))

> * whether it should always include the fractional part (in this
> example it doesn't make any sense)

Indeed, it probably doesn't make sense unless the number is < 100.
I would also put that information inside parentheses, as it is redundant with the per-loop timing.

> * maybe it should say "loops/s" rather than just "/s"

Yeah.
msg197303 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-08 17:59
I'm afraid the adding any separators will make some people angry. With a comma or space it no more a valid number in Python and many other languages and can't be copy/pasted and parsed.

Actually I sometimes use small shell scripts to run "python -m timeit" and parsing results with grep, sed and awk. It is easer in simplest cases than writing it on Python (especially when different Python binaries used).

I'm -0,1.
msg197305 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-08 18:02
I'm with Serhiy on this.  So if separators are added, I would say they *must* be optional.  Presumably if they are added they should be locale dependent :)  All of which may well make it more complicated than it is worth.
msg197308 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-09-08 18:22
-1 from me, and I'm a comma-loving American ;-)

I'm sure lots of code in the wild parses this output - Serhiy isn't the only one doing it.
msg197314 - (view) Author: Jakub Stasiak (jstasiak) * Date: 2013-09-08 19:48
To me the point of this patch is adding number of loops per second information - using thousands separators was just an addition which I'm happy to drop.

Please find attached 2 patches:
- one containing actual changes - loops per second added, fractional part is printed when the number is less than 100, space is used as thousands separator; old part of the message is not affected
- one containing PEP8 reformatting

So:

    10000 loops, best of 3: 34.6 usec per loop (28 870 loops/s)
msg197315 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-08 19:50
> So:
> 
>     10000 loops, best of 3: 34.6 usec per loop (28 870 loops/s)

Looks fine to me (though having a decimal space at the right and not at
the left looks a bit weird).
msg197316 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-08 20:04
I do not find a space to be an acceptable separator, sorry.
msg197852 - (view) Author: Jakub Stasiak (jstasiak) * Date: 2013-09-16 01:17
Antoine: I agree that it does look weird to have thousands separators at one place and not at the other but IMO it's still slightly better - the number formatted with separators is simply more readable that separator-less one.

R. David Murray: what's the acceptable separator then? Modifying it to be locale-aware would make it inconsistent with the rest of the output (which, admittedly, is also the case with introducing space separator, dot still is the fraction separator though).

I modified my patch to not have the separators at all, please find timeit-v3-actual-changes-no-separators.patch - if that's the condition for accepting this patch then so be it - the description of the patch would be "print number of loops per second" and the example output is:

10000 loops, best of 3: 34.6 usec per loop (28870 loops/s)
msg197858 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-16 01:42
My vote is no separators.  But I'm just one vote :)
msg197869 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-16 05:42
> My vote is no separators.  But I'm just one vote :)

Seconded.
msg197878 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-16 08:29
I'm fine with the change too. Tim, what do you think?

Speaking of which, some examples were really done with an old machine:

    $ python -m timeit 'try:' '  str.__bool__' 'except AttributeError:' '  pass'
-   100000 loops, best of 3: 15.7 usec per loop
+   1000000 loops, best of 3: 0.701 usec per loop (1425528 loops/s)

:-)
msg197882 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-16 08:51
Inconsistency: "sec per loop" vs. "loops/s".
msg197885 - (view) Author: Jakub Stasiak (jstasiak) * Date: 2013-09-16 09:54
That's right, I was actually wondering about it few minutes before you pointed it out. Find new patch attached.
msg300916 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-08-27 13:46
Reviewing this patch, it appears that the PEP8 changes to timeit.py are already in the source and the discussion of the thousands separator is no longer an issue with the underscore changes in 3.6 (meaning underscore now seems the way to separate digits in large numbers, although it seems the vote was to remove all separators from timeit anyway).

That would leave the `loops per second` request the only one still needed from this patch.  But, with the subsequent change of selecting the time unit in timeit, perhaps this patch would need to be modified to print 'loops per {timeunit}'?
History
Date User Action Args
2017-08-27 13:46:09cheryl.sabellasetnosy: + cheryl.sabella
messages: + msg300916
2013-09-16 09:54:48jstasiaksetfiles: + timeit-v4-actual-changes.patch

messages: + msg197885
2013-09-16 08:51:36serhiy.storchakasetmessages: + msg197882
2013-09-16 08:29:31pitrousetmessages: + msg197878
2013-09-16 05:42:25serhiy.storchakasetmessages: + msg197869
2013-09-16 01:42:59r.david.murraysetmessages: + msg197858
2013-09-16 01:17:50jstasiaksetfiles: + timeit-v3-actual-changes-no-separators.patch

messages: + msg197852
2013-09-08 20:04:19r.david.murraysetmessages: + msg197316
2013-09-08 19:50:05pitrousetmessages: + msg197315
2013-09-08 19:48:27jstasiaksetfiles: + timeit-v3-pep8.patch
2013-09-08 19:48:10jstasiaksetfiles: + timeit-v3-actual-changes.patch

messages: + msg197314
2013-09-08 18:22:16tim.peterssetmessages: + msg197308
2013-09-08 18:02:56r.david.murraysetnosy: + r.david.murray
messages: + msg197305
2013-09-08 17:59:03serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg197303
2013-09-08 17:03:01pitrousetnosy: + pitrou
messages: + msg197287
2013-09-08 16:53:03jstasiaksetmessages: + msg197286
2013-09-08 16:41:53ezio.melottisetnosy: + ezio.melotti

messages: + msg197282
stage: patch review
2013-09-08 16:35:47pitrousetnosy: + tim.peters
2013-09-08 16:16:11jstasiaksetfiles: + timeit-v2.patch

messages: + msg197276
2013-09-08 16:00:31jstasiakcreate