brennerm / PyTricks

Collection of less popular features and tricks for the Python programming language
MIT License
3.07k stars 502 forks source link

Add a shorter way to find the indexes of min and max elements of a list. #98

Open askanium opened 5 years ago

flintforge commented 5 years ago

I don't see the benefit of this. Looks like an "anti-trick" to me, that shouldn't be used.

benchmark :

from minmaxindex import *
L = [x for x in range(3000000)]

%alias_magic t timeit 
%t minIndex(L)
%t maxIndex(L)
%t L.index(min(L)) 
%t L.index(max(L))

And for fair results, one should

import random
random.shuffle(L)

before trying again

askanium commented 5 years ago

I will respectfully disagree.

First, it seems the timing is not consistent and for your solution you used -n 10 while mine was ran only once and thus didn't average on results of several runs. I ran it several times and got my solution perform faster around 40-50% times.

Second, on smaller arrays, using list.index() performs way faster:

l = list(range(10000))
random.shuffle(l)
%t -n 100 minIndex(l)  # 555 µs ± 51.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
random.shuffle(l)
%t -n 100 l1.index(min(l))  # 160 µs ± 3.79 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Third, in the repo readme, you have this bit in the Intention block: "Creating a knowledge base of unpopular Python built-in features to save a lot of unnecessary code." And this in the requirements:

However, only performance is discussed here. If this is the most important factor, it would be great to have the README reflect it then.

flintforge commented 5 years ago

By default, the number of loops of timeit is based on a heuristic, so that the duration of the test be long enough (around 0.2sec), before pulling the best timing, not an average.

Of course, L.index(min(L)) is very likely to perform faster than any other implementation, especially those written in python and trying to achieve the same result. And that was my point :

You stated the correct way to do so, I wasn't criticizing the PR, but the proposed trick by @huwenchao Using min(iterator, key) can be very useful, but not for the minimum element of a list.

askanium commented 5 years ago

@flintforge, one usually doesn't comment regarding existing merged code, stating it shouldn't be used and not saying a word about the code in PR. Your comment above was clearly stated regarding my solution: "I don't see the benefit of this. Looks like an "anti-trick" to me, that shouldn't be used." Now you are saying it's the right way to do it. And it's not @huwenchao who added the trick. It was @st0le, but that doesn't really matter, as you merged it.