-
-
Notifications
You must be signed in to change notification settings - Fork 46.6k
Unify O(sqrt(N))
is_prime
functions under project_euler
#6258
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
Unify O(sqrt(N))
is_prime
functions under project_euler
#6258
Conversation
I think it is better to remove the assertion in all together since it doesn't have actual value for the implementation of the primality test algorithm and maybe it is a bit of confusing even. Should I do that? @poyea Edit: For example in project_euler/problem_003/sol1.py lines 41-43. |
@elpaxoudis Yeah, I think they can be removed, and the negative / type check can be performed when processing input (if any). If we want to handle the negative / type check in the function, I think it makes sense to raise an exception / an assertion failure, instead of return |
is_prime
under project_euler
O(sqrt(N))
is_prime
under project_euler
O(sqrt(N))
is_prime
under project_euler
O(sqrt(N))
is_prime
functions under project_euler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your pull request!🤩
Co-authored-by: John Law <[email protected]>
@poyea Is there any other changes I should make? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while and this looks good to me. 👍
Describe your change:
I changed the implementation of is_prime functions inside project_euler in order to have a unified implementation. There are some cases where the solution uses the Eratosthenes' sieve method. In some cases there is no specific gain from using that method so I changed it to the O(sqrt(n)) algorithm, but in other cases the sieve method is used in the core structure of the solution, hence I did not touch those.
Checklist:
Fixes: #{$ISSUE_NO}
.fixes #5434