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.

Sign up for GitHub

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Added support for i32, i64, f32 and f64's sqrt and cbrt #2571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
87415bf
Added Support for real and integer sqrt and cbrt
Kishan-Ved Mar 3, 2024
6b11066
updated
Kishan-Ved Mar 3, 2024
f5c8b3d
updated
Kishan-Ved Mar 3, 2024
2e34731
added integration test
Kishan-Ved Mar 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions integration_tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

Copy link
Collaborator

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.

Copy link
Contributor Author

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

RUN(NAME test_pass_compare LABELS cpython llvm c)
RUN(NAME test_c_interop_01 LABELS cpython llvm c)
RUN(NAME test_c_interop_02 LABELS cpython llvm c
Expand Down
36 changes: 36 additions & 0 deletions integration_tests/test_math_04.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from math import (cbrt, sqrt)
from lpython import f32, f64, i32, i64

eps: f64
eps = 1e-12

def test_cbrt():
eps: f64 = 1e-12
a : i32 = 64
b : i64 = i64(64)
c : f32 = f32(64.0)
d : f64 = f64(64.0)
assert abs(cbrt(124.0) - 4.986630952238646) < eps
Copy link
Collaborator

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).

Copy link
Contributor Author

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.

assert abs(cbrt(39.0) - 3.3912114430141664) < eps
assert abs(cbrt(39) - 3.3912114430141664) < eps
assert abs(cbrt(a) - 4.0) < eps
assert abs(cbrt(b) - 4.0) < eps
assert abs(cbrt(c) - 4.0) < eps
assert abs(cbrt(d) - 4.0) < eps

def test_sqrt():
eps: f64 = 1e-12
a : i32 = 64
b : i64 = i64(64)
c : f32 = f32(64.0)
d : f64 = f64(64.0)
assert abs(sqrt(a) - 8.0) < eps
assert abs(sqrt(b) - 8.0) < eps
assert abs(sqrt(c) - 8.0) < eps
assert abs(sqrt(d) - 8.0) < eps

def check():
test_cbrt()
test_sqrt()

check()
56 changes: 56 additions & 0 deletions src/runtime/math.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,18 +539,74 @@ def trunc(x: f32) -> i32:
else:
return ceil(x)

@overload
def sqrt(x: f32) -> f64:
Copy link
Collaborator

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?

Copy link
Collaborator

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().

Copy link
Collaborator

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

"""
Returns square root of a number x
"""
y : f64
y = f64(x)
return y**(1/2)

@overload
def sqrt(x: f64) -> f64:
"""
Returns square root of a number x
"""
return x**(1/2)

@overload
def sqrt(x: i32) -> f64:
"""
Returns square root of a number x
"""
y : f64
y = float(x)
return y**(1/2)

@overload
def sqrt(x: i64) -> f64:
"""
Returns square root of a number x
"""
y : f64
y = float(x)
return y**(1/2)

@overload
def cbrt(x: f32) -> f64:
"""
Returns cube root of a number x
"""
y : f64
y = f64(x)
return y**(1/3)

@overload
def cbrt(x: f64) -> f64:
"""
Returns cube root of a number x
"""
return x**(1/3)

@overload
def cbrt(x: i32) -> f64:
"""
Returns cube root of a number x
"""
y : f64
y = float(x)
return y**(1/3)

@overload
def cbrt(x: i64) -> f64:
"""
Returns cube root of a number x
"""
y : f64
y = float(x)
return y**(1/3)

@ccall
def _lfortran_dsin(x: f64) -> f64:
pass
Expand Down
Loading