nv-legate / cunumeric

An Aspiring Drop-In Replacement for NumPy at Scale
https://docs.nvidia.com/cunumeric/24.06/
Apache License 2.0
620 stars 70 forks source link

Support 2d array copies that use advanced indexing #12

Open piyueh opened 3 years ago

piyueh commented 3 years ago

Problem

When using a boolean array to access elements of another array (with a non-trivial size), it raises an error:

TypeError: nonzero() missing 1 required positional argument: 'stacklevel'

To reproduce

  1. step 1: create test.py with the following code:
    from legate import numpy
    qw = numpy.random.random((100, 100))
    qw[qw < 0.3] = 1.0
  2. step 2: run test.py with legate, e.g.,
    $ legate --cpus 1 ./test.py

Output

Traceback (most recent call last):
  File "<blahblah>/python3.8/site-packages/legion_top.py", line 394, in legion_python_main
    run_path(args[start], run_name='__main__')
  File "<blahblah>/lib/python3.8/site-packages/legion_top.py", line 193, in run_path
    exec(code, module.__dict__, module.__dict__)
  File "./test.py", line 3, in <module>
    qw[qw < 0.3] = 1.0
  File "<blahblah>/lib/python3.8/site-packages/legate/numpy/array.py", line 753, in __setitem__
    self._thunk.set_item(key, value_array._thunk, stacklevel=2)
  File "<blahblah>/lib/python3.8/site-packages/legate/numpy/deferred.py", line 487, in set_item
    index_array = self._create_indexing_array(
  File "<blahblah>/lib/python3.8/site-packages/legate/numpy/deferred.py", line 324, in _create_indexing_array
    tuple_of_arrays = key.nonzero()
TypeError: nonzero() missing 1 required positional argument: 'stacklevel'

Expected output

Either it works or raises a NotImplementedError so that users know it has not yet been implemented.

Notes

lightsighter commented 3 years ago

I pushed a partial fix for this one. It will get you to a NotImplementedError, where we need a task to take the arrays returned by nonzero and convert them into a field containing points for indexing. @manopapad come talk to me before working further on this one.

piyueh commented 3 years ago

Thanks! Feel free to close this issue or use it to keep tracking the issue (if you will implement it in the future).

lightsighter commented 3 years ago

We'll leave it open for now. It is a feature that should be implemented.

manopapad commented 3 years ago

I added support for advanced indexing on 2d arrays at https://github.com/manopapad/legate.numpy/tree/slicing. I am waiting to merge it until I get some comments on how the changes conflict with updates that @magnatelee is working on. In the meantime, @piyueh could you please try out the above branch and let me know if it works on your original code that used advanced slicing?

piyueh commented 3 years ago

@manopapad Do you mean the example code in this issue? I tested the PR #42 with the example code in this issue and got the following error:

[0 - 7f4efcdd7700]    1.047198 {6}{python}: python exception occurred within task:
Traceback (most recent call last):
  File "<prefix>/lib/python3.8/site-packages/legion_top.py", line 408, in legion_python_main
    run_path(args[start], run_name='__main__')
  File "<prefix>/lib/python3.8/site-packages/legion_top.py", line 200, in run_path
    exec(code, module.__dict__, module.__dict__)
  File "./test.py", line 3, in <module>
    qw[qw < 0.3] = 1.0
  File "<prefix>/lib/python3.8/site-packages/legate/numpy/array.py", line 742, in __setitem__
    self._thunk.set_item(key, value_array._thunk, stacklevel=2)
  File "<prefix>/lib/python3.8/site-packages/legate/numpy/deferred.py", line 613, in set_item
    raise NotImplementedError(
NotImplementedError: Singleton arrays in advanced indexing 
piyueh commented 3 years ago

On the other hand, if you mean the advanced indexing in my application code, I got another kind of error with that PR. Here's the Gist that mimics my application code: https://gist.github.com/piyueh/e68d2ae7eb8d562442258705cae31da4 I ran it with legate --cpus 1 ./legate --cpus 1 ./advanced_indexing_test.py -lg:numpy:test and got the following error:

[0 - 7fab50ba3700]    1.068933 {4}{runtime}: [warning 1071] LEGION WARNING: Region requirement 1 of operation
Copy (UID 26) in parent task legion_python_main (UID 1) is using uninitialized data for field(s) 1048577 of
logical region (7,1,7) (from file <prefix>/legion/runtime/legion/legion_ops.cc:1200)
For more information see: http://legion.stanford.edu/messages/warning_code.html#warning_code_1071

./advanced_indexing_test.py:40: UserWarning: Legate performing implicit type conversion from uint64 to float64
  j, i = j[i != 0], i[i != 0]
[0 - 7fab50785700]    1.089844 {6}{python}: python exception occurred within task:
Traceback (most recent call last):
  File "<prefix>/lib/python3.8/site-packages/legion_top.py", line 408, in legion_python_main
    run_path(args[start], run_name='__main__')
  File "<prefix>/lib/python3.8/site-packages/legion_top.py", line 200, in run_path
    exec(code, module.__dict__, module.__dict__)
  File "./advanced_indexing_test.py", line 40, in <module>
    j, i = j[i != 0], i[i != 0]
  File "<prefix>/lib/python3.8/site-packages/legate/numpy/array.py", line 375, in __getitem__
    shape=None, thunk=self._thunk.get_item(key, stacklevel=2)
  File "<prefix>/lib/python3.8/site-packages/legate/numpy/deferred.py", line 477, in get_item
    raise NotImplementedError("views with advanced indexing")
NotImplementedError: views with advanced indexing

Nevertheless, I think the test in this Gist is different from the use case discussed in this issue, though they are both advanced indexing. Do we have an issue for the advanced indexing used in this Gist post?

manopapad commented 3 years ago

I have updated this branch https://github.com/manopapad/legate.numpy/tree/slicing with the required support for the latest example you shared (https://gist.github.com/piyueh/e68d2ae7eb8d562442258705cae31da4). When you get the chance, please try it out (and your main code if possible). Note that, as I wrote in #50, legate.numpy may list the indices of nonzero elements in a different order than python NumPy, so these lines and these lines should be commented out, but the rest of the code should work.

Singleton arrays will still error out, but as long as you can work without this feature I'd rather let it sit for now. Some upcoming changes to legate.core will make singleton arrays much easier to handle, and I'd rather wait for those to land.