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

Create maths/pi_generator.py #8666

Merged
merged 20 commits into from
Apr 18, 2023
Merged

Create maths/pi_generator.py #8666

merged 20 commits into from
Apr 18, 2023

Conversation

JulianStiebler
Copy link
Contributor

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 include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed require tests Tests [doctest/unittest/pytest] are required require type hints https://docs.python.org/3/library/typing.html labels Apr 17, 2023
Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Apr 17, 2023
Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Apr 17, 2023
Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

@algorithms-keeper algorithms-keeper bot added tests are failing Do not merge until tests pass and removed tests are failing Do not merge until tests pass labels Apr 17, 2023
@algorithms-keeper algorithms-keeper bot removed the require type hints https://docs.python.org/3/library/typing.html label Apr 17, 2023
Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

@JulianStiebler
Copy link
Contributor Author

Kay all tests seem fine. Do i need to add a unittest myself?

@JulianStiebler JulianStiebler changed the title New Algorithm for Python/maths/pi_generator.py Create python/maths/pi_generator.py Apr 17, 2023
@JulianStiebler JulianStiebler changed the title Create python/maths/pi_generator.py Create maths/pi_generator.py Apr 17, 2023
Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

@JulianStiebler
Copy link
Contributor Author

JulianStiebler commented Apr 18, 2023

Hi! Thanks for your responses.

  • added commentary as recommended by @rohan472000
  • added math.pi comparison & math.isclose()-test as recommended by @cclauss
  • added some more commentary since our output will be a str() and to use in math-functions, we need to convert to float. (Theres also a type-hint on calculate_pi() tho but just for clearness i also wrote down in the docstring
  • I didn't remove the test against 50 and 100 preknown values of pi since the constant from math.pi is only 16 digits
  • I also didn't touched the main-function call

Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

Copy link

@algorithms-keeper algorithms-keeper bot left a comment

Choose a reason for hiding this comment

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

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

  • @algorithms-keeper review to trigger the checks for only added pull request files
  • @algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Let's land this one! The only other thing that I could think of doing would be to see how low the tolerances on math.isclose() could be set and still pass the tests but that is clearly not vital.

Thanks for your contribution!

@algorithms-keeper algorithms-keeper bot removed the awaiting reviews This PR is ready to be reviewed label Apr 18, 2023
@cclauss cclauss merged commit 1158294 into TheAlgorithms:master Apr 18, 2023
@JulianStiebler
Copy link
Contributor Author

JulianStiebler commented Apr 18, 2023


>>> math.isclose(float(calculate_pi(15)), math.pi, abs_tol = 0)
True

Trying:
    math.isclose(float(calculate_pi(15)), math.pi, abs_tol = 0)
Expecting:
    True
ok

>>> math.isclose(float(calculate_pi(50)), math.pi, abs_tol = 0.1)
True

Trying:
    math.isclose(float(calculate_pi(50)), math.pi, abs_tol = 0.1)
Expecting:
    True
ok

>>> math.isclose(float(calculate_pi(50)), math.pi, abs_tol = 0)
True

Trying:
    math.isclose(float(calculate_pi(50)), math.pi, abs_tol = 0)
Expecting:
    True
ok

Is that what you meant?
Thanks for your review!

Can i still commit it with those changed tests or how does it go from here when its merged?

@cclauss
Copy link
Member

cclauss commented Apr 18, 2023

My sense is that when abs_tol = 0, the function goes with its default of 9 decimal places.

Perhaps if abs_tol was set to 0.00000000000000000000000001 then tests would start to fail.

@JulianStiebler
Copy link
Contributor Author

JulianStiebler commented Apr 18, 2023

>>>math.isclose(float(calculate_pi(100)), math.pi, abs_tol = 0.0000000000000001)
True

Trying:
    math.isclose(float(calculate_pi(50)), math.pi, abs_tol = 0.0000000000000001)
Expecting:
    True
ok

Since the constant only delivers 15 digits,
but for completeness:

    >>> math.isclose(float(calculate_pi(50)), float(3.1415926535897932384626433832795028841971693993751058209749445923078164062862089986280348253421170679), abs_tol = 0.00000000000000000000000000000000000001)
    True


Trying:
    math.isclose(float(calculate_pi(50)), float(3.1415926535897932384626433832795028841971693993751058209749445923078164062862089986280348253421170679), abs_tol = 0.00000000000000000000000000000000000001)
Expecting:
    True
ok

Since we didnt interrupted the series but rather finished it with a limit it should always match pi to the exact digit we chose
I also tested it with abs_tol = 0.000000000000000000000000000000000000000000000001 (50 digits excluding the .) on calculate_pi(50) and it passed too. The arbitrary taken Pi-number is not from the algorithm! I searched google for 100digits of pi and compared 2-3 different ones (were all the same but to make sure :D)

@cclauss
Copy link
Member

cclauss commented Apr 18, 2023

Nice work! Thanks.

tianyizheng02 pushed a commit to tianyizheng02/Python that referenced this pull request May 29, 2023
* Create pi_generator.py

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pi_generator.py

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Updated commentary on line 28, added math.pi comparison & math.isclose() test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Removed  # noqa: E501

* printf() added as recommended by cclaus

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
sedatguzelsemme pushed a commit to sedatguzelsemme/Python that referenced this pull request Sep 15, 2024
* Create pi_generator.py

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pi_generator.py

* Update pi_generator.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Updated commentary on line 28, added math.pi comparison & math.isclose() test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Removed  # noqa: E501

* printf() added as recommended by cclaus

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@isidroas isidroas mentioned this pull request Jan 25, 2025
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require tests Tests [doctest/unittest/pytest] are required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants