-
Notifications
You must be signed in to change notification settings - Fork 167
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 support for i32, i64, f32 and f64's sqrt and cbrt #2571
base: main
Are you sure you want to change the base?
Conversation
@Kishan-Ved I had done some research on the I request you to implement them. 🙂 |
Yes @kmr-srbh , I've heard about them. My only concern is that these methods (like Newton-Raphson) give approximate solutions. We need the error to be lesser than 1e-12 from the actual square root (at least as per the current rigour of the tests). If we implement these methods, we will need a large number of iterations to get this precision (and there might be cases when we never achieve it). Also, python's floating point precision might cause some errors. Nevertheless, I'll see if these can be implemented, thanks! :) |
You are right @Kishan-Ved. Newton-Raphson method was just an example. It is not ideal for our use case. In fact it resorts to infinite loops for some numbers! I am excited for what you implement! :) |
@@ -626,6 +626,7 @@ RUN(NAME test_builtin_sum LABELS cpython llvm c) | |||
RUN(NAME test_math1 LABELS cpython llvm c) | |||
RUN(NAME test_math_02 LABELS cpython llvm NOFAST) | |||
RUN(NAME test_math_03 LABELS llvm) #1595: TODO: Test using CPython (3.11 recommended) | |||
RUN(NAME test_math_04 LABELS llvm) #TODO: Test using CPython (as above) |
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.
Why not enable CPython for this test? Does it fail with CPython?
I think a test should run with CPython so that we are sure that we are supporting and delivering the right syntax and runtime output.
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.
The test fails when I enable CPython. Here is the error:
Traceback (most recent call last):
File "/home/kishan/Desktop/lpython/integration_tests/test_math_04.py", line 1, in <module>
from math import (cbrt, sqrt)
ImportError: cannot import name 'cbrt' from 'math' (/home/kishan/anaconda3/envs/lp/lib/python3.10/lib-dynload/math.cpython-310-x86_64-linux-gnu.so)
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.
Is it because CPython does not have this implemented?
The math module has cbrt(), according to the official python documentation: https://docs.python.org/3/library/math.html.
Is this what I'm supposed to refer to? I couldn't find CPython documentation.
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.
I get the same error when I enable CPython for test_math_03 (which has already been merged).
Traceback (most recent call last):
File "/home/kishan/Desktop/lpython/integration_tests/test_math_03.py", line 1, in <module>
from math import (cbrt, exp2)
ImportError: cannot import name 'cbrt' from 'math' (/home/kishan/anaconda3/envs/lp/lib/python3.10/lib-dynload/math.cpython-310-x86_64-linux-gnu.so)
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.
It seems cbrt
in python is supported from version 3.11
. I think we currently use python 3.10
locally and at the CI. Hence, you get the above import error.
I think it is fine for now to not enable the test for CPython.
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.
Ok. I face the same issue here too, please check: #2556
@@ -539,18 +539,74 @@ def trunc(x: f32) -> i32: | |||
else: | |||
return ceil(x) | |||
|
|||
@overload | |||
def sqrt(x: f32) -> f64: |
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.
Should this return a f64
or an f32
? Similarly for sqrt(x: i32)
, should it return a f64
or an f32
?
It seems we might need some discussion here. I do not yet have an opinion about the type of the return value that we should return. @certik Do you have any ideas on this?
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.
Similar to the return value of sqrt()
that we choose/decide, we should implement/update cbrt()
.
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.
I think we have to return f32, see the following the cases.
>>> import numpy as n
>>> x = n.float32(3.14)
>>> type(x)
<class 'numpy.float32'>
>>> type(n.sqrt(x))
<class 'numpy.float32'>
>>> y = 3.14
>>> type(y)
<class 'float'>
>>> import math as m
>>> type(m.sqrt(y))
<class 'float'>
If we move this implement to IntrinsicFunction in the future, this can be utilised by fortran as well, as it returns the same: https://gcc.gnu.org/onlinedocs/gfortran/SQRT.html
I think these But for the moment, the current approach in this PR also seems fine to me. |
b : i64 = i64(64) | ||
c : f32 = f32(64.0) | ||
d : f64 = f64(64.0) | ||
assert abs(cbrt(124.0) - 4.986630952238646) < eps |
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.
We can also print the values before each assert so that it is helpful for debugging later (if the test case fails for some reason).
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.
Ok, I'll follow this practice from now on.
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.
I shared couple comments above. The implementation seems fine to me over short term, but the functions should be implemented using intrinsic function registry for the long term.
I have doubts over the return value that we should use for the sqrt()
and cbrt()
functions supported in this PR.
I think this should not be merged until https://github.com/lcompilers/lpython/pull/2571/files#r1515144790 is clarified.
Please mark this PR ready for review once it is ready. |
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.
The changes seem fine to me for the short term. For the long term we need these to be implemented as intrinsic registry functions.
We need to discuss/decide on the return values for sqrt
and cbrt
in case of i32
and f32
arguments.
We have sqrt implemented in the intrinsic function registry. How do we redirect the control to it? |
In ASR we represent Now, I think we have to do the same for cbrt as well. |
Till now, LPython's math module has sqrt() and cbrt() functions that work only for f64 types. In this PR, I have added support for i32, i64, f32 and f64 datatypes, along with an integration test.