-
Notifications
You must be signed in to change notification settings - Fork 45
Enable global lists within kernels #584
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
base: main
Are you sure you want to change the base?
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Co-authored-by: Graham Markall <[email protected]>
|
/ok to test |
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
| if ty is None: | ||
| raise ValueError(f"Cannot type list element type {type(val[0])}") | ||
| return types.List(ty, reflected=True) | ||
| return ty[::1] |
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.
This was a sketch of how things work for a 1D list only. A more robust implementation would:
- Check that the purpose of the typeof operation is for a constant. We should not accept lists as arguments to kernels.
- Convert the value to a NumPy array, then determine the Numba type of the NumPy array. This will allow handling of lists-of-lists, e.g.
[[1, 2], [3, 4]]. It also prevents having to handle what happens if there are different element types (e.g. int and float), whereas the existing implementation assumes that all list elements have the same type, which need not be the case. - Not check for zero-length, because a zero-length NumPy array is possible and can be typed. The inability to handle a zero-length list is an issue of the reflected list implementation in Numba that doesn't present an issue for freezing global lists as arrays.
gmarkall
left a comment
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.
A couple of other additions that should be required:
- Documentation. Not only is this a deviation from Python semantics (in terms of freezing the list as a constant), it even deviates from Numba list handling so could be surprising to a seasoned Numba user.
- Additional testing: test with multiple data types in the list, an unfreezable list (e.g. one that won't convert to a NumPy array), test with a list of lists once the implementation of
_typeof_listis modified according to comments.
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.
I think the changes here are now superfluous?
Closes #580