Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

askanium
Copy link
Contributor

No description provided.

@flintforge
Copy link
Contributor

flintforge commented Jun 10, 2019

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
Copy link
Contributor Author

askanium commented Jun 10, 2019

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.

@flintforge
Copy link
Contributor

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants