PyTricks icon indicating copy to clipboard operation
PyTricks copied to clipboard

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

Open askanium opened this issue 5 years ago • 4 comments

askanium avatar Jun 10 '19 07:06 askanium

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

flintforge avatar Jun 10 '19 17:06 flintforge

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.

askanium avatar Jun 10 '19 18:06 askanium

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 avatar Jun 10 '19 19:06 flintforge

@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.

askanium avatar Jun 11 '19 05:06 askanium