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

Kishan-Ved · 2024-03-03T09:52:09Z

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.

kmr-srbh · 2024-03-03T11:51:24Z

@Kishan-Ved I had done some research on the sqrt() method and found that raising a number to the power (1/2) is not computationally efficient for calculating square roots of large numbers . There are other efficient methods like the Newton-Raphson method.

I request you to implement them. 🙂

Kishan-Ved · 2024-03-03T18:05:25Z

@Kishan-Ved I had did some research on the sqrt() method and found that raising a number to the power (1/2) is not computationally efficient for calculating square roots of large numbers . There are other efficient methods like the Newton-Raphson method.

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! :)

kmr-srbh · 2024-03-03T18:52:46Z

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! :)

Shaikh-Ubaid · 2024-03-06T20:57:34Z

integration_tests/CMakeLists.txt

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


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.

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)

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.

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)

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.

Ok. I face the same issue here too, please check: #2556

Shaikh-Ubaid · 2024-03-06T21:03:03Z

src/runtime/math.py

@@ -539,18 +539,74 @@ def trunc(x: f32) -> i32:
    else:
        return ceil(x)

+@overload
+def sqrt(x: f32) -> f64:


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?

Similar to the return value of sqrt() that we choose/decide, we should implement/update cbrt().

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

Shaikh-Ubaid · 2024-03-06T21:08:58Z

I think these sqrt() and cbrt() functions should ideally be implemented as a part of the intrinsic registry functions.

But for the moment, the current approach in this PR also seems fine to me.

Shaikh-Ubaid · 2024-03-06T21:10:52Z

integration_tests/test_math_04.py

+    b : i64 = i64(64)
+    c : f32 = f32(64.0)
+    d : f64 = f64(64.0)
+    assert abs(cbrt(124.0) - 4.986630952238646) < eps


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

Ok, I'll follow this practice from now on.

Shaikh-Ubaid

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.

Thirumalai-Shaktivel · 2024-03-07T06:45:03Z

Please mark this PR ready for review once it is ready.

Shaikh-Ubaid

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.

Kishan-Ved · 2024-03-07T13:49:03Z

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?

Thirumalai-Shaktivel · 2024-03-09T07:28:39Z

In ASR we represent sqrt as IntrinsicScalarFunction, then it is redirected to IntrinsicFunctionSqrt for float types in instantiate_Sqrt in the intrinsic_function_registry.h file. As LLVM provides llvm.f32.sqrt intrinsic support, we compute the output using that.

Now, I think we have to do the same for cbrt as well.

Kishan-Ved added 4 commits March 3, 2024 15:18

Added Support for real and integer sqrt and cbrt

87415bf

updated

6b11066

updated

f5c8b3d

added integration test

2e34731

Shaikh-Ubaid reviewed Mar 6, 2024

View reviewed changes

Shaikh-Ubaid requested changes Mar 6, 2024

View reviewed changes

Thirumalai-Shaktivel marked this pull request as draft March 7, 2024 06:45

Shaikh-Ubaid reviewed Mar 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Kishan-Ved commented Mar 3, 2024

kmr-srbh commented Mar 3, 2024 •

edited

Loading

Kishan-Ved commented Mar 3, 2024

kmr-srbh commented Mar 3, 2024

Shaikh-Ubaid Mar 6, 2024

Kishan-Ved Mar 7, 2024

Kishan-Ved Mar 7, 2024

Kishan-Ved Mar 7, 2024

Shaikh-Ubaid Mar 7, 2024

Kishan-Ved Mar 7, 2024

Shaikh-Ubaid Mar 6, 2024

Shaikh-Ubaid Mar 6, 2024

Thirumalai-Shaktivel Mar 9, 2024

Shaikh-Ubaid commented Mar 6, 2024

Shaikh-Ubaid Mar 6, 2024

Kishan-Ved Mar 7, 2024

Shaikh-Ubaid left a comment

Thirumalai-Shaktivel commented Mar 7, 2024

Shaikh-Ubaid left a comment

Kishan-Ved commented Mar 7, 2024 •

edited

Loading

Thirumalai-Shaktivel commented Mar 9, 2024 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

Kishan-Ved commented Mar 3, 2024

kmr-srbh commented Mar 3, 2024 • edited Loading

Kishan-Ved commented Mar 3, 2024

kmr-srbh commented Mar 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

Thirumalai-Shaktivel commented Mar 7, 2024

Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

Kishan-Ved commented Mar 7, 2024 • edited Loading

Thirumalai-Shaktivel commented Mar 9, 2024 • edited Loading

kmr-srbh commented Mar 3, 2024 •

edited

Loading

Kishan-Ved commented Mar 7, 2024 •

edited

Loading

Thirumalai-Shaktivel commented Mar 9, 2024 •

edited

Loading