Trig functions (Fixes: #2000) #5865
Conversation
ghost
added
require descriptive names
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
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 reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto 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.
| # A lot of the maclaurin series require | ||
| # a factorial in their expressions. This | ||
| # is just a simple O(n) algo for factorial | ||
| def factorial(x: int) -> int: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file maths/trig_functions.py, please provide doctest for the function factorial
Please provide descriptive name for the parameter: x
|
|
||
|
|
||
| def sine( | ||
| x: float, |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
| return sum | ||
|
|
||
| def cosine( | ||
| x: float, |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
|
|
||
| return sum | ||
|
|
||
| def tangent(x: float) -> float: |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
| except ZeroDivisionError: | ||
| pass | ||
|
|
||
| def cosecant(x: float) -> float: |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
| pass | ||
|
|
||
| def secant( | ||
| x: float, |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
| except ZeroDivisionError: | ||
| pass | ||
|
|
||
| def cotangent(x: float) -> float: |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
|
As for the factorial problem: I thought since it is out of the scope of the algorithms that are being focused on here, and only exists as a helper function, doctests was unnecessary. As for the descriptive parameter name problem: x = input into a function. This is a common practice within mathematics in this context and I believe the x is plenty descriptive within the context of these functions. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
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 reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto 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.
| # A lot of the maclaurin series require | ||
| # a factorial in their expressions. This | ||
| # is just a simple O(n) algo for factorial | ||
| def factorial(x: int) -> int: |
There was a problem hiding this comment.
As there is no test file in this pull request nor any test function or class in the file maths/trig_functions.py, please provide doctest for the function factorial
Please provide descriptive name for the parameter: x
|
|
||
|
|
||
| def sine( | ||
| x: float, |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
| return sum | ||
|
|
||
| def cosine( | ||
| x: float, |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
|
|
||
| return sum | ||
|
|
||
| def tangent(x: float) -> float: |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
| except ZeroDivisionError: | ||
| pass | ||
|
|
||
| def cosecant(x: float) -> float: |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
| pass | ||
|
|
||
| def secant( | ||
| x: float, |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
| except ZeroDivisionError: | ||
| pass | ||
|
|
||
| def cotangent(x: float) -> float: |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: x
2 participants
Describe your change:
I added the 6 trigonometric functions. For sin and cos I used a maclaurin series approximations. For tan, csc, sec, and cot, I used trig identities and my aforementioned sin and cos algorithm. No external library (including the math library that ships with python) was used. I did use
mathso that I could use a precise value of pi, but this only served for accurate testing of the algorithms and did not actually factor into the algorithms themselves.I think the cosine algorithm needs some work, it has a variance around 1E-15 currently but I am unsure of how to improve the accuracy (there are some pythonic limits to how far I can take an infinite series. Namely an infinite series which contains a factorial).
Additionally, I think there should also be the inverse trigonometric functions in this repository but I am not currently aware of an algorithm and it might be better saved for a separate pull request
Checklist:
Fixes: #{$ISSUE_NO}.