PyTricks
PyTricks copied to clipboard
Add a shorter way to find the indexes of min and max elements of a list.
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
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:
- compared to the "general approach":
- improve readability (yes)
- improve performance (yes, for small lists and sometimes for big as well)
- implement functionality in a shorter way (yes)
However, only performance is discussed here. If this is the most important factor, it would be great to have the README reflect it then.
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.
@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.