Skip to content

rename and add doctest #1501

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

Merged
merged 1 commit into from
Oct 28, 2019
Merged

rename and add doctest #1501

merged 1 commit into from
Oct 28, 2019

Conversation

realDuYuanChao
Copy link
Member

@@ -1,16 +1,21 @@
# NguyenU
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is out of sync with master. There are already doctests on this file:
https://github.com/TheAlgorithms/Python/blob/master/maths/find_max.py#L6

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cclauss Please review again!

@cclauss
Copy link
Member

cclauss commented Oct 28, 2019

Please write a https://docs.python.org/3/library/timeit.html that proves that the new implementation is faster than the current implementation. My bet is that the function call and slice overhead means that it is not.

@realDuYuanChao
Copy link
Member Author

Please write a https://docs.python.org/3/library/timeit.html that proves that the new implementation is faster than the current implementation. My bet is that the function call and slice overhead means that it is not.

Did you mean below code implementation is slower ?

    max_num = nums[0]
    for i in range(1, len(nums)):
        if nums[i] > max_num:
            max_num = nums[i]

@cclauss
Copy link
Member

cclauss commented Oct 28, 2019

Not sure but that is my suspicion but a timeit test can tell us for sure.

@realDuYuanChao
Copy link
Member Author

After I tested using

def find_max(nums):
    max_num = nums[0]
    for i in range(1, len(nums)):
        if nums[i] > max_num:
            max_num = nums[i]
    return max_num
print(timeit.timeit("find_max([2, 4, 9, 7, 19, 94, 5])", setup="from __main__ import find_max", number=10000))
time: 0.022231400000000002
def find_max(nums):
    max = nums[0]
    for x in nums:
        if x > max:
            max = x
    return max
print(timeit.timeit("find_max([2, 4, 9, 7, 19, 94, 5])", setup="from __main__ import find_max", number=10000))
time: 0.007430200000000005

From the result, You are right. Thanks

@cclauss
Copy link
Member

cclauss commented Oct 28, 2019

This is awesome work. I think we should fill this file with as many different implementations of find_max() as we can think of with timeit tests for each one in an effort to find that fastest pure Python implementation.

How does print(timeit.timeit("max([2, 4, 9, 7, 19, 94, 5])", number=10000)) perform using the Python builtin?

@realDuYuanChao
Copy link
Member Author

print(timeit.timeit("max([2, 4, 9, 7, 19, 94, 5])", number=10000))

0.004652999999999997 , Very fast


>>> find_min([1, 3, 5, 7, 9]) == 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the same tests data as find_max.py uses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Please review again

@realDuYuanChao realDuYuanChao changed the title add doctest and optimization rename and add doctest Oct 28, 2019
Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Thanks for your persistence.

@cclauss cclauss merged commit 3fc276c into TheAlgorithms:master Oct 28, 2019
@realDuYuanChao realDuYuanChao deleted the patch-2 branch November 3, 2019 13:22
stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
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