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

Added solution to Project Euler 69 #2934

Merged
merged 7 commits into from
Oct 11, 2020

Conversation

sarthaka1310
Copy link
Contributor

Describe your change:

For #hacktoberfest
Added solution to Project Euler 69 for issue #2695

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

project_euler/problem_69/solution_69.py Outdated Show resolved Hide resolved
project_euler/problem_69/solution_69.py Outdated Show resolved Hide resolved
project_euler/problem_69/solution_69.py Outdated Show resolved Hide resolved
project_euler/problem_69/solution_69.py Outdated Show resolved Hide resolved
@dhruvmanila dhruvmanila added awaiting changes A maintainer has requested changes to this PR require tests Tests [doctest/unittest/pytest] are required labels Oct 11, 2020
sarthaka1310 and others added 2 commits October 11, 2020 13:24
Co-authored-by: Dhruv <dhruvmanila@gmail.com>
@sarthaka1310
Copy link
Contributor Author

@dhruvmanila Implemented the requested changes, please review.
If found acceptable, please add the hacktoberfest-accepted label.

"""

if n <= 0:
raise ValueError("Please enter a natural number")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise ValueError("Please enter a natural number")
raise ValueError("Please enter a natural number greater than 0")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Natural numbers are greater than 0 by definition. If you still want it, I'll commit :)

Copy link
Member

Choose a reason for hiding this comment

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

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?

project_euler/problem_69/sol1.py Outdated Show resolved Hide resolved
Comment on lines +51 to +55
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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Member

Choose a reason for hiding this comment

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

That line always evaluates to True so it's not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After I accepted this, the test failed. Are you sure of this?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I'm curious as to how did you not realize that your code would become incorrect without the if statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 dhruvmanila added hacktoberfest-accepted Accepted to be counted towards Hacktoberfest and removed require tests Tests [doctest/unittest/pytest] are required labels Oct 11, 2020
Co-authored-by: Dhruv <dhruvmanila@gmail.com>
@TravisBuddy
Copy link

Travis tests have failed

Hey @sarthaka1310,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: e9a2f7c0-0be5-11eb-a59b-f3f3774a9991

@dhruvmanila dhruvmanila merged commit d02f6bb into TheAlgorithms:master Oct 11, 2020
stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
* 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>
Panquesito7 pushed a commit to Panquesito7/Python that referenced this pull request May 13, 2021
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes A maintainer has requested changes to this PR hacktoberfest-accepted Accepted to be counted towards Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants