Added solution to Project Euler 69 - #2934
Conversation
dhruvmanila
added
awaiting changes
Co-authored-by: Dhruv <dhruvmanila@gmail.com>
| """ | ||
|
|
||
| if n <= 0: | ||
| raise ValueError("Please enter a natural number") |
There was a problem hiding this comment.
| raise ValueError("Please enter a natural number") | |
| raise ValueError("Please enter a natural number greater than 0") |
There was a problem hiding this comment.
Natural numbers are greater than 0 by definition. If you still want it, I'll commit :)
There was a problem hiding this comment.
Yes, please, or maybe write something like the input value must be greater than 0 and please put the if statement back as that was a mistake on my part. Did you not realize that your code would become incorrect without the if statement?
| for number in range(2, n + 1): | ||
| if phi[number] == number: | ||
| phi[number] -= 1 | ||
| for multiple in range(number * 2, n + 1, number): | ||
| phi[multiple] = (phi[multiple] // number) * (number - 1) |
There was a problem hiding this comment.
| for number in range(2, n + 1): | |
| if phi[number] == number: | |
| phi[number] -= 1 | |
| for multiple in range(number * 2, n + 1, number): | |
| phi[multiple] = (phi[multiple] // number) * (number - 1) | |
| for number in range(2, n + 1): | |
| phi[number] -= 1 | |
| for multiple in range(number * 2, n + 1, number): | |
| phi[multiple] = (phi[multiple] // number) * (number - 1) |
There was a problem hiding this comment.
That line always evaluates to True so it's not needed.
There was a problem hiding this comment.
After I accepted this, the test failed. Are you sure of this?
There was a problem hiding this comment.
This was a mistake on my part. I did not see that you were changing the later elements of the list in the nested for loop. Please change it back.
There was a problem hiding this comment.
I'm curious as to how did you not realize that your code would become incorrect without the if statement?
There was a problem hiding this comment.
I did know what happened but gave you the benefit of doubt. Here's an explanation of what went wrong:
With the edit you made, the loop did not identify primes anymore and executed this for all numbers instead, therefore affecting the ratio.
There was a problem hiding this comment.
@dhruvmanila I have another open PR#2868 and would appreciate it if you could review that too. (Also, it uses the sieve creation similar to the phi constructed here)
There was a problem hiding this comment.
As mentioned, only submit one PR at a time. I have a lot to go through so that might take days. Thank you for your cooperation.
dhruvmanila
added
hacktoberfest-accepted
Co-authored-by: Dhruv <dhruvmanila@gmail.com>
* Added solution to Project Euler 69 * Accept edits from code review Co-authored-by: Dhruv <dhruvmanila@gmail.com> * Added doctests * Renaming and exception handling * Apply suggestions from code review Co-authored-by: Dhruv <dhruvmanila@gmail.com> * Edited mistake. Co-authored-by: formal-acc <sarthak.agrawal@research.iiit.ac.in> Co-authored-by: Dhruv <dhruvmanila@gmail.com>
* Added solution to Project Euler 69 * Accept edits from code review Co-authored-by: Dhruv <dhruvmanila@gmail.com> * Added doctests * Renaming and exception handling * Apply suggestions from code review Co-authored-by: Dhruv <dhruvmanila@gmail.com> * Edited mistake. Co-authored-by: formal-acc <sarthak.agrawal@research.iiit.ac.in> Co-authored-by: Dhruv <dhruvmanila@gmail.com>
4 participants
Describe your change:
For #hacktoberfest
Added solution to Project Euler 69 for issue #2695
Checklist:
Fixes: #{$ISSUE_NO}.