-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fixing issue 7693 #7712
Fixing issue 7693 #7712
Conversation
This also fixes the other reproducer from 7693 import numpy as np
from numba import njit
dest_dtype = np.dtype([("array1", np.int16, (3,))], align=True)
@njit
def ex1(dest):
tmp = (np.arange(3) + 1).astype(np.int16)
dest['array1'] = tmp
dest = np.empty(5, dtype=dest_dtype)
expected = np.empty(5, dtype=dest_dtype)
ex1(dest[0])
ex1.py_func(expected[0])
print(expected[0])
print(dest[0]) which gives:
with this PR. |
Thanks for looking at this so far! I hope you don't mind that I pushed a fix for the flake8 issues, just to make it a little clearer where the "true" failures are. |
Note that the failures are all variations on the same issue:
where |
� Conflicts: � numba/np/arrayobj.py
oh, I didn't see the message and added one too. I'll remove it later. For now, I'm trying to fix the tests that the previous push broke. Some of them are passing locally, I hope I've fixed all of them. |
open question for a reviewer:
|
gpuci run tests |
@luk-f-a Many thanks for the fixes - from a preliminary glance this is looking promising - do you consider this non-draft / ready for review now?
I think it's as good as we can do given the way the
I think it's better to keep it where it is because some code sets attrs on structured types rather than using |
thanks for the feedback, I'll remove the duplicated test and promote to non-draft |
@gmarkall do the new tests need a cuda version? |
Ah, that's a good point - I'm not a sure if this woudl work with regualr arrays in CUDA, so I#ll have a quick check to see. |
gpuci run tests |
It turns out that the new tests do work in CUDA, so I've added them. |
The lack of a py_func in the FakeCUDAKernel is a slightly surprising omission, although it must have been missed for a long time as many CUDA tests have to be skipped with the simulator. Since it brings the API into line with the hardware target and fixes a record dtype test without requiring any complex / invasive changes, we add it here.
gpuci run tests |
@stuartarchibald Many thanks for the review and suggested fixes. I also thought this might fail assigning an array to a whole array's worth of nested arrays, but it fails in typing as required, so I've just added an additional test to ensure that we continue to produce a typing error in this case (instead of it sneaking past typing and miscompiling or raising a lowering error. I've also merged from master, but there were no conflicts. |
gpuci run tests |
The gpuCI fail should be resolved when #7740 is merged. |
gpuci run tests |
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.
Thanks for the patch and fixes, looks good.
@gmarkall please could you review/approve too RE patch 364fe7b from #7712 (comment) as I'm author. Thanks. |
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.
Approved re patch 364fe7b, of which @stuartarchibald is the author.
Thanks @gmarkall |
|
all green |
Thanks @esc |
Fixes the lowering of setitem of a nestedarray into the nestedarray field of a record. Changes to the generated LLVM IR in those cases, to prevent the metadata (strides, etc) of the source being copied into the data buffer of the destination array.
fixes #7693