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.

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

Fix regression #7355: cannot set items in structured array data types #7428

Merged
merged 3 commits into from Sep 22, 2021

Conversation

sklam
Copy link
Member

@sklam sklam commented Sep 21, 2021

This is a minimal fix for #7355 and does not replace #7359. We need a minimal fix for 0.54.1

Edit, adding details:

This is the easiest fix because:

  • making the base Array.copy() return a covariant type is the reason of the regression for nestedarray.
  • The new array subclass feature can override copy() to specialize accordingly.
  • Requiring copy() is less invasive and array subclass support is new and experimental.

This change will require array subclass to override copy().

@sklam sklam changed the title Fix regression #7355. Cannot set items in structured array data types Fix regression #7355: cannot set items in structured array data types Sep 21, 2021
@esc
Copy link
Member

esc commented Sep 22, 2021

Probably Fixes #7426 too.

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch. I've tested it with this... on HEAD of mainline with the diff to the tests that this patch introduces, the corresponding test fails. Later this PR is checked out and the same runs fine:

$ git rev-parse HEAD
ed6ae6d3c2a615d329604c96e034efc876cf8230

$ git diff
diff --git a/numba/tests/test_record_dtype.py b/numba/tests/test_record_dtype.py
index 8f1137f..699017e 100644
--- a/numba/tests/test_record_dtype.py
+++ b/numba/tests/test_record_dtype.py
@@ -315,6 +315,11 @@ def set_field4(rec):
     return rec
 
 
+def set_field_slice(arr):
+    arr['k'][:] = 0.0
+    return arr
+
+
 recordtype = np.dtype([('a', np.float64),
                        ('b', np.int16),
                        ('c', np.complex64),
@@ -865,6 +870,10 @@ class TestRecordDtype(unittest.TestCase):
         cfunc = self.get_cfunc(pyfunc, (nbrecord,))
         self.assertEqual(cfunc(rec), pyfunc(rec))
 
+        pyfunc = set_field_slice
+        cfunc = self.get_cfunc(pyfunc, (nbrecord,))
+        np.testing.assert_array_equal(cfunc(rec), pyfunc(rec))
+
     def test_structure_dtype_with_titles(self):
         # the following is the definition of int4 vector type from pyopencl
         vecint4 = np.dtype([(('x', 's0'), 'i4'), (('y', 's1'), 'i4'),

$ ./runtests.py numba.tests.test_record_dtype.TestRecordDtype.test_record_two_arrays
E
======================================================================
ERROR: test_record_two_arrays (numba.tests.test_record_dtype.TestRecordDtype)
Tests that comparison of NestedArrays by key is working correctly. If
----------------------------------------------------------------------
Traceback (most recent call last):
<snip>
numba.core.errors.TypingError: No implementation of function Function(<built-in function setitem>) found for signature:
 
 >>> setitem(nestedarray(int32, (10, 20)), slice<a:b>, float64)
 
There are 16 candidate implementations:
  - Of which 14 did not match due to:
  Overload of function 'setitem': File: <numerous>: Line N/A.
    With argument(s): '(nestedarray(int32, (10, 20)), slice<a:b>, float64)':
   No match.
  - Of which 2 did not match due to:
  Overload in function 'SetItemBuffer.generic': File: numba/core/typing/arraydecl.py: Line 171.
    With argument(s): '(nestedarray(int32, (10, 20)), slice<a:b>, float64)':
   Rejected as the implementation raised a specific error:
     TypeError: __init__() got an unexpected keyword argument 'ndim'

$ #git stash/checkout this PR etc.

$ git rev-parse HEAD
1c3c10a528fb3e5ca0071886af49cc700a811577

$ ./runtests.py numba.tests.test_record_dtype.TestRecordDtype.test_record_two_arrays
.
----------------------------------------------------------------------
Ran 1 test in 0.584s

OK

I've also manually tested this PR against all the examples in #7355 and #7426, and those reported here: https://numba.discourse.group/t/883/7

Comment on lines +455 to +456
return Array(dtype=dtype, ndim=ndim, layout=layout, readonly=readonly,
aligned=self.aligned)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review labels Sep 22, 2021
@sklam sklam merged commit 306060a into numba:master Sep 22, 2021
@sklam sklam deleted the fix/iss7355_minimal branch September 22, 2021 17:34
sklam added a commit to sklam/numba that referenced this pull request Sep 22, 2021
Fix regression numba#7355: cannot set items in structured array data types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge Effort - short Short size effort needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants