Added is identity comparison for numpy records and added tests as is_comparison_tests.py #9384

Kamakshi8104 · 2024-01-09T08:52:08Z

Hello!

I have added the workaround to numba/np/arrayobj.py and have created test cases for the same.
Please let me know if any more changes are required
regards,
Kamakshi

gmarkall

Many thanks for the PR. I've posted some initial feedback to address - once that's addressed, we may need an additional round of review on the tests (I haven't gone further commenting on the test cases as there already some issues there).

gmarkall · 2024-01-09T08:56:37Z

numba/np/arrayobj.py

+def test_kernel(a, b) -> None:
+    print(int(a is a))
+    print(int(a is b))
+    print(int(b is a))
+    print(int(b is b))
+
+
+a = np.zeros(1, dtype=[("a", "i4"), ("b", "i4")])[0]
+b = a.copy()
+njit(test_kernel)(a, b)
+cuda.jit(test_kernel)[1, 1](a, b)
+cuda.synchronize()
+


The workaround is the lowering implementation record_is, not all the code that went with it to demonstrate that it worked correctly.

Suggested change

def test_kernel(a, b) -> None:

print(int(a is a))

print(int(a is b))

print(int(b is a))

print(int(b is b))

a = np.zeros(1, dtype=[("a", "i4"), ("b", "i4")])[0]

b = a.copy()

njit(test_kernel)(a, b)

cuda.jit(test_kernel)[1, 1](a, b)

cuda.synchronize()

gmarkall · 2024-01-09T09:00:07Z

numba/np/arrayobj.py

@@ -41,6 +41,28 @@
                                       _choose_concatenation_layout)


+@lower_builtin(operator.is_, types.Record, types.Record)


Rather than inserting this lowering right at the top of the file, the natural place for it would be in the Comparisons section, next to the implementation of array_is.

gmarkall · 2024-01-09T09:02:15Z

numba/np/arrayobj.py

@@ -29,7 +29,7 @@
 from numba.core.typing import signature
 from numba.core.types import StringLiteral
 from numba.core.extending import (register_jitable, overload, overload_method,
-                                  intrinsic, overload_attribute)
+                                  intrinsic, overload_attribute, lower_builtin)


lower_builtin was already imported.

Suggested change

intrinsic, overload_attribute, lower_builtin)

intrinsic, overload_attribute)

gmarkall · 2024-01-09T09:03:24Z

numba/np/arrayobj.py

@@ -12,7 +12,7 @@

 import numpy as np

-from numba import pndindex, literal_unroll
+from numba import pndindex, literal_unroll, cuda, njit, types #type : ignore


types was already imported. cuda cannot be imported in Numba outside of the numba.cuda module in general.

Suggested change

from numba import pndindex, literal_unroll, cuda, njit, types #type : ignore

from numba import pndindex, literal_unroll

gmarkall · 2024-01-09T09:04:28Z

numba/np/is_comparison_tests.py

@@ -0,0 +1,75 @@
+import numpy as np


Whilst it's good to have added tests, they should be in the numba.tests module, and should fit in with the existing unit test framework.

gmarkall · 2024-01-09T09:04:52Z

numba/np/is_comparison_tests.py

+    a = np.zeros(1, dtype=[("a", "i4"), ("b", "i4")])[0]
+    b = a.copy()
+
+    result = cuda.jit(test_kernel)[1, 1](a, b)


CUDA kernels cannot return values.

gmarkall · 2024-01-09T09:05:34Z

numba/np/is_comparison_tests.py

+    a = np.zeros(1, dtype=[("a", "i4"), ("b", "i4")])[0]
+    b = a.copy()
+
+    result = test_kernel(a, b)


test_kernel returns None in its present implementation (which needs removing from numba.np.arrayobj also).

Kamakshi8104

Hey ...thank you for the feedback! I have incorporated changes mentioned by you . If anything else still needs to be done please let me know 🙂

gmarkall · 2024-01-10T11:05:24Z

Thanks for making changes. For future changes - please could you avoid force-pushing as this tends to confuse the Github review interface - many thanks in advance!

gmarkall · 2024-01-10T11:11:26Z

Having skimmed over the latest revision, I'm not going to be able to spend more time reviewing this PR in its current form - I think it would be helpful for you to receive additional mentorship to fill in some of the conceptual gaps here, and that's not something I have enough time to do.

If you'd like to proceed with the PR, I would suggest making a post on the Numba Discourse (https://numba.discourse.group) describing what you're trying to achieve with the PR, and requesting if there's anyone with sufficient time and experience to help shepherd the PR closer to completion and being ready for another round of review.

Kamakshi8104 · 2024-01-10T17:41:45Z

Hey,
I want to proceed with the PR and will connect with someone to help me complete it. Thanks!

numba/np/arrayobj.py

numba/tests/is_comparison_tests.py

numba/np/arrayobj.py

…ay_methods.py

Kamakshi8104 · 2024-01-19T21:02:05Z

I have made the corrections as pointed out by @guilhermeleobas . Sorry for the delay 🙂

…ay_methods.py

numba/tests/test_array_methods.py

numba/np/arrayobj.py

Kamakshi8104 · 2024-01-24T12:12:03Z

Thanks for the help @guilhermeleobas .
I have made changes please let me know if there are any more problems.
I had to use from np.arrayobj import record_is in test_array_methods.py since all the checks were failing without it

guilhermeleobas

Thanks for the contributions, @Kamakshi8104. Code still needs some work to be done.

numba/tests/test_array_methods.py

numba/np/arrayobj.py

Kamakshi8104 · 2024-01-25T08:59:12Z

Thanks @guilhermeleobas ! I have tried a different approach for record_is_impl function. I hope this works!! Please update me on any needed changes🙂

guilhermeleobas

Hi @Kamakshi8104, I made a mistake while reviewing your PR. The code you had originally should work.

Thanks, @gmarkall, for pointing that out.

numba/np/arrayobj.py

Kamakshi8104 · 2024-01-26T17:29:07Z

@guilhermeleobas I have changed it back as requested. Please take a look....let me know If anything else is required

guilhermeleobas

Thanks for the effort @Kamakshi8104. Have a few more comments, but we're getting there.

guilhermeleobas · 2024-02-02T15:38:58Z

numba/tests/test_array_methods.py

@@ -11,6 +11,7 @@
 from numba.core.errors import TypingError, NumbaValueError
 from numba.np.numpy_support import as_dtype, numpy_version
 from numba.tests.support import TestCase, MemoryLeakMixin, needs_blas
+#from np.arrayobj import record_is


Delete this import as it is not needed

guilhermeleobas · 2024-02-02T15:41:04Z

numba/tests/test_array_methods.py

+    def test_record_is(self):
+        def check(a, b, expected):
+            pyfunc = identity_usecase
+            cfunc=njit(pyfunc)
+
+
+            result = cfunc(a, b)
+            self.assertEqual(result, expected)
+
+        record_type = np.dtype([("a", "i4"), ("b", "i4")])
+
+        # Test case: Same records
+        a = np.zeros(1, dtype=record_type)[0]
+        b = a.copy()
+        check(a, b, True)
+
+        # Test case: Different records
+        c = np.ones(1, dtype=record_type)[0]
+        check(a, c, False)
+
+        # Test case: Same records with different values
+        d = np.zeros(1, dtype=record_type)[0]
+        d['a'] = 42
+        check(a, d, False)


Rather than give the expected result as an argument, compute it by calling pyfunc(a, b). This should fix the CI issue.

Suggested change

def test_record_is(self):

def check(a, b, expected):

pyfunc = identity_usecase

cfunc=njit(pyfunc)

result = cfunc(a, b)

self.assertEqual(result, expected)

record_type = np.dtype([("a", "i4"), ("b", "i4")])

# Test case: Same records

a = np.zeros(1, dtype=record_type)[0]

b = a.copy()

check(a, b, True)

# Test case: Different records

c = np.ones(1, dtype=record_type)[0]

check(a, c, False)

# Test case: Same records with different values

d = np.zeros(1, dtype=record_type)[0]

d['a'] = 42

check(a, d, False)

def test_record_is(self):

def check(a, b):

pyfunc = identity_usecase

cfunc = njit(pyfunc)

result = cfunc(a, b)

expected = pyfunc(a, b)

self.assertEqual(result, expected)

record_type = np.dtype([("a", "i4"), ("b", "i4")])

# Test case: Same records

a = np.zeros(1, dtype=record_type)[0]

b = a.copy()

check(a, b)

# Test case: Different records

c = np.ones(1, dtype=record_type)[0]

check(a, c)

# Test case: Same records with different values

d = np.zeros(1, dtype=record_type)[0]

d['a'] = 42

check(a, d)

guilhermeleobas · 2024-02-02T15:41:35Z

numba/tests/test_array_methods.py

+
+
+
+    # tests for record  comparison
+


Do you plan to add any other tests? If not, remove the empty lines.

Kamakshi8104 · 2024-02-02T19:37:25Z

Thank you @guilhermeleobas and @gmarkall for all your help!! All the tests have successfully passed. I hope this fixes the issue🙂

guilhermeleobas

Oh, you need to also write a release notes document. Please, see:
https://github.com/numba/numba/blob/main/docs/upcoming_changes/README.rst

Kamakshi8104 · 2024-03-04T14:59:03Z

@gmarkall and @guilhermeleobas I have made all changes as well as added documentation. Please let me know if anything else I required further since the PR hasn't been merged yet. 🙂

docs/upcoming_changes/9384.new_feature.rst

Co-authored-by: Guilherme Leobas <guilhermeleobas@gmail.com>

docs/upcoming_changes/9384.new_feature.rst

Co-authored-by: Guilherme Leobas <guilhermeleobas@gmail.com>

guilhermeleobas

If @gmarkall does not have any other comments, we can merge this one for 0.61

added is identity comparison for numpy records and added tests

d07525e

gmarkall linked an issue Jan 9, 2024 that may be closed by this pull request

numpy records do not support "is" identity comparison. #9382

Open

gmarkall requested changes Jan 9, 2024

View reviewed changes

gmarkall added the 4 - Waiting on author Waiting for author to respond to review label Jan 9, 2024

made required changes to initial pull request

c114da6

Kamakshi8104 commented Jan 9, 2024

View reviewed changes

Kamakshi8104 force-pushed the is-comparison branch from a746223 to c114da6 Compare January 10, 2024 06:36

gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jan 10, 2024

gmarkall added 4 - Waiting on author Waiting for author to respond to review PR Help needed Help is needed to complete a PR. and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jan 10, 2024

guilhermeleobas reviewed Jan 12, 2024

View reviewed changes

numba/np/arrayobj.py Outdated Show resolved Hide resolved

numba/tests/is_comparison_tests.py Outdated Show resolved Hide resolved

numba/np/arrayobj.py Outdated Show resolved Hide resolved

adding record_is to correct position and adding testcases to test_arr…

d1074d5

…ay_methods.py

Kamakshi8104 added 3 commits January 20, 2024 11:58

adding record_is to correct position and adding testcases to test_arr…

78becf3

…ay_methods.py

minor changes for failing tests

2fe60cc

fixing flake8 errors

259c628

guilhermeleobas requested changes Jan 24, 2024

View reviewed changes

numba/tests/test_array_methods.py Outdated Show resolved Hide resolved

numba/tests/test_array_methods.py Outdated Show resolved Hide resolved

numba/tests/test_array_methods.py Outdated Show resolved Hide resolved

numba/np/arrayobj.py Show resolved Hide resolved

Kamakshi8104 added 3 commits January 24, 2024 16:47

making required changes

018f6ce

making required changes

18e5ae5

making required changes

dffe2ee

guilhermeleobas requested changes Jan 24, 2024

View reviewed changes

numba/tests/test_array_methods.py Outdated Show resolved Hide resolved

numba/tests/test_array_methods.py Outdated Show resolved Hide resolved

numba/tests/test_array_methods.py Outdated Show resolved Hide resolved

numba/np/arrayobj.py Outdated Show resolved Hide resolved

making required corrections to comparison and tests

72885ef

guilhermeleobas requested changes Jan 26, 2024

View reviewed changes

numba/np/arrayobj.py Outdated Show resolved Hide resolved

making required corrections

109335c

guilhermeleobas requested changes Feb 2, 2024

View reviewed changes

Kamakshi8104 added 2 commits February 3, 2024 00:23

making required changes to added testcases

90e3a1c

making required changes to added testcases

f7f42ea

guilhermeleobas added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Feb 9, 2024

guilhermeleobas approved these changes Feb 9, 2024

View reviewed changes

guilhermeleobas requested changes Feb 9, 2024

View reviewed changes

Kamakshi8104 added 2 commits February 17, 2024 01:56

added release notes

505a1cd

added release notes

cea5418

guilhermeleobas requested changes Mar 4, 2024

View reviewed changes

docs/upcoming_changes/9384.new_feature.rst Outdated Show resolved Hide resolved

Update docs/upcoming_changes/9384.new_feature.rst

957a0d4

Co-authored-by: Guilherme Leobas <guilhermeleobas@gmail.com>

guilhermeleobas reviewed Mar 15, 2024

View reviewed changes

docs/upcoming_changes/9384.new_feature.rst Outdated Show resolved Hide resolved

Update docs/upcoming_changes/9384.new_feature.rst

a08a8ae

Co-authored-by: Guilherme Leobas <guilhermeleobas@gmail.com>

guilhermeleobas added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Apr 29, 2024

guilhermeleobas approved these changes Apr 29, 2024

View reviewed changes

guilhermeleobas added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 5 - Ready to merge Review and testing done, is ready to merge labels Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added is identity comparison for numpy records and added tests as is_comparison_tests.py #9384

Added is identity comparison for numpy records and added tests as is_comparison_tests.py #9384

Kamakshi8104 commented Jan 9, 2024

gmarkall left a comment

gmarkall Jan 9, 2024

gmarkall Jan 9, 2024

gmarkall Jan 9, 2024

gmarkall Jan 9, 2024

gmarkall Jan 9, 2024

gmarkall Jan 9, 2024

gmarkall Jan 9, 2024

Kamakshi8104 left a comment

gmarkall commented Jan 10, 2024

gmarkall commented Jan 10, 2024

Kamakshi8104 commented Jan 10, 2024

Kamakshi8104 commented Jan 19, 2024

Kamakshi8104 commented Jan 24, 2024 •

edited

Loading

guilhermeleobas left a comment

Kamakshi8104 commented Jan 25, 2024

guilhermeleobas left a comment

Kamakshi8104 commented Jan 26, 2024 •

edited

Loading

guilhermeleobas left a comment

guilhermeleobas Feb 2, 2024

guilhermeleobas Feb 2, 2024

guilhermeleobas Feb 2, 2024

Kamakshi8104 commented Feb 2, 2024

guilhermeleobas left a comment

Kamakshi8104 commented Mar 4, 2024

guilhermeleobas left a comment

		@@ -41,6 +41,28 @@
		_choose_concatenation_layout)


		@lower_builtin(operator.is_, types.Record, types.Record)

	intrinsic, overload_attribute, lower_builtin)
	intrinsic, overload_attribute)

	from numba import pndindex, literal_unroll, cuda, njit, types #type : ignore
	from numba import pndindex, literal_unroll

Added is identity comparison for numpy records and added tests as is_comparison_tests.py #9384

Are you sure you want to change the base?

Added is identity comparison for numpy records and added tests as is_comparison_tests.py #9384

Conversation

Kamakshi8104 commented Jan 9, 2024

gmarkall left a 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

Kamakshi8104 left a comment

Choose a reason for hiding this comment

gmarkall commented Jan 10, 2024

gmarkall commented Jan 10, 2024

Kamakshi8104 commented Jan 10, 2024

Kamakshi8104 commented Jan 19, 2024

Kamakshi8104 commented Jan 24, 2024 • edited Loading

guilhermeleobas left a comment

Choose a reason for hiding this comment

Kamakshi8104 commented Jan 25, 2024

guilhermeleobas left a comment

Choose a reason for hiding this comment

Kamakshi8104 commented Jan 26, 2024 • edited Loading

guilhermeleobas left a 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

Kamakshi8104 commented Feb 2, 2024

guilhermeleobas left a comment

Choose a reason for hiding this comment

Kamakshi8104 commented Mar 4, 2024

guilhermeleobas left a comment

Choose a reason for hiding this comment

Kamakshi8104 commented Jan 24, 2024 •

edited

Loading

Kamakshi8104 commented Jan 26, 2024 •

edited

Loading