added support for 'searchsorted'#414
Conversation
cunumeric/array.py
Outdated
| Multiple GPUs, Multiple CPUs | ||
| """ | ||
|
|
||
| v = convert_to_cunumeric_ndarray(v) |
There was a problem hiding this comment.
any reason that you don't use add_boilerplate?
There was a problem hiding this comment.
also, this statement changes the type of v, which makes the typing harder. if you want to keep this statement, I suggest you change the variable name on the LHS.
There was a problem hiding this comment.
I used this statement in order to guarantee to pass the argument as a ndarray further - even for scalar inputs. I have changed the lhs variable name and added a comment to clarify.
|
|
||
| def check_dtypes(): | ||
| np.random.seed(42) | ||
| check_api(generate_random((156,), np.uint8)) |
There was a problem hiding this comment.
consider using pytest decorators instead of enumerating test calls like this. e.g., https://github.com/nv-legate/cunumeric/blob/branch-22.07/tests/integration/test_matmul.py#L28-L29
|
|
||
|
|
||
| def compare_assert(a_np, a_num): | ||
| if not num.allclose(a_np, a_num): |
There was a problem hiding this comment.
a nitpick: can't this be array_equal? I think searchsorted shouldn't ever do anything that could cause rounding errors
| { | ||
| auto input = input_array.read_accessor<VAL, 1>(rect_base); | ||
| auto input_v = input_values.read_accessor<VAL, 1>(rect_values); | ||
| assert(input.accessor.is_dense_arbitrary(rect_base)); |
There was a problem hiding this comment.
please request an exact instance for the input to make this assertion hold all the time
|
@mfoerste4 doing a full search in every iteration seems pretty bad for memory bandwidth. do you know of any optimized algorithms that search for multiple values simultaneously? at minimum we could sort the search values to improve the access locality. |
I will bring this question up in the next tech-meeting. @ipdemes , could you do a second pass review? |
|
@mfoerste4 there are some corner cases I think we would like to test explicitly. (I think you do test them by generating inputs randomly, but, maybe, we would like to force these tests ): Some more tests that I believe are not tested: |
|
another comment for the test: |
|
|
||
| Availability | ||
| -------- | ||
| Multiple GPUs, Multiple CPUs |
There was a problem hiding this comment.
this is not required in the docstring of the cunuemeric/array,py file, only at the cunumeric/module.py file
There was a problem hiding this comment.
I have seen it in some places in array.py as well (some of them are from myself though). Should I remove it? Just Availability or the whole doc?
There was a problem hiding this comment.
@bryevdv : you have done the most recent changes in the way documentation/comparison table is built. Do we need to have Availability section in array.py? Or having it in module.py is enough?
|
@ipdemes , I think I have addressed all of your suggestions. Please have another look. Thanks. |
CPU/OMP & GPU support for searchsorted
The (local) GPU implementation is kept simple for now, using a single thread for every value to-be-inserted performing binary search. AFAIU this is how cupy does it. It might make sense to do something more sophisticated here.