-
Notifications
You must be signed in to change notification settings - Fork 170
cbrt and exp2 functions implemented #564
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
Conversation
…cosh written" revert commit This reverts commit 41551aa.
@Madhav2310 you should try to setup LPython locally and simply test this locally before pushing. You can then iterate very quickly, instead of waiting for the CI to finish. |
|
||
pi: f64 = 3.141592653589793238462643383279502884197 | ||
e: f64 = 2.718281828459045235360287471352662497757 | ||
tau: f64 = 6.283185307179586 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this level of precision for testing? I think 4-5 decimal places is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately these constants should be imported from the builtin math module.
""" | ||
Returns cube root of a number x | ||
""" | ||
return x**(1/3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these expressions actually work with Lpython?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, they should.
@Smit-create Could you please see why it is failing to import the functions in the test files? |
@Madhav2310 -- you should merge this with the latest |
@namannimmo10 My local repository is up to date, could you elaborate on what to do exactly? |
pi: f64 = 3.141592653589793238462643383279502884197 | ||
e: f64 = 2.718281828459045235360287471352662497757 | ||
tau: f64 = 6.283185307179586 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please try removing these global variables and try running tests with local variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I'll do that
Are these functions actually available in CPython's math? If not then it will definitely raise an error. |
I don't see these functions here: https://docs.python.org/3/library/math.html |
Right, I just saw it. It is very weird, they have tests for these functions, but not the functions themselves https://github.com/python/cpython/blob/2d787971c65b005d0cce219399b9a8e2b70d4ef4/Lib/test/test_math.py |
Assuming that you've added remote $ git checkout main
$ git pull upstream main
$ git checkout madhav
$ git rebase main Now, if you run the integration tests, they'll pass. |
Well, that won't pass for sure. >>> from math import exp2
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ImportError: cannot import name 'exp2' from 'math' (/Users/thebigbool/opt/anaconda3/envs/lp/lib/python3.10/lib-dynload/math.cpython-310-darwin.so) |
See the test carefully, they have tested for
|
My bad. Yes, they won't pass because of this. |
I see, thank you! I'll take care of it in the future |
No description provided.