Skip to content

rename and add doctest - #1501

Merged
cclauss merged 1 commit into
TheAlgorithms:masterfrom
realDuYuanChao:patch-2
Oct 28, 2019
Merged

rename and add doctest #1501
cclauss merged 1 commit into
TheAlgorithms:masterfrom
realDuYuanChao:patch-2

Conversation

@realDuYuanChao

Copy link
Copy Markdown
Member

Comment threadmaths/find_max.py
@@ -1,16 +1,21 @@
# NguyenU

Copy link
Copy Markdown
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
Copy Markdown
MemberAuthor

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

cclauss commented Oct 28, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

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]
foriinrange(1, len(nums)):
ifnums[i] >max_num:
max_num=nums[i]

@cclauss

Copy link
Copy Markdown
Member

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

@realDuYuanChao

Copy link
Copy Markdown
MemberAuthor

After I tested using

deffind_max(nums):
max_num=nums[0]
foriinrange(1, len(nums)):
ifnums[i] >max_num:
max_num=nums[i]
returnmax_numprint(timeit.timeit("find_max([2, 4, 9, 7, 19, 94, 5])", setup="from __main__ import find_max", number=10000))
time: 0.022231400000000002
deffind_max(nums):
max=nums[0]
forxinnums:
ifx>max:
max=xreturnmaxprint(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
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

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

0.004652999999999997 , Very fast

Comment threadmaths/find_min.py Outdated
:param nums: contains elements
:return: max number in list

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

Copy link
Copy Markdown
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
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Updated. Please review again

@realDuYuanChaorealDuYuanChao changed the title add doctest and optimizationrename and add doctest Oct 28, 2019

@cclausscclauss left a comment

Copy link
Copy Markdown
Member

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:masterOct 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 freeto 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

@realDuYuanChao@cclauss