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

Advanced indexing #235

Merged
merged 36 commits into from
Apr 21, 2022
Merged

Advanced indexing #235

merged 36 commits into from
Apr 21, 2022

Conversation

ipdemes
Copy link
Contributor

@ipdemes ipdemes commented Mar 25, 2022

Adding support for advanced indexing

@ipdemes ipdemes requested a review from manopapad March 25, 2022 04:03
@ipdemes ipdemes changed the base branch from branch-22.03 to branch-22.05 March 25, 2022 15:12
cunumeric/runtime.py Outdated Show resolved Hide resolved
test.py Outdated Show resolved Hide resolved
cunumeric/config.py Outdated Show resolved Hide resolved
@manopapad
Copy link
Contributor

Note that, when deciding where to put the extra dimensions in a partial-basic, partial-advanced expression, NumPy considers constants as advanced arrays, so if it sees something like x[:,ind,:,1] it considers it two advanced arrays separated by :, and puts the added dimensions in the beginning:

import numpy as np
import cunumeric as cn
def foo(lib):
    x = lib.ones((3,4,5,6))
    ind = lib.ones((2,2), dtype=int)
    return x[:,ind,:,1].shape
print(foo(np))
print(foo(cn))

prints:

(2, 2, 3, 5)
(3, 2, 2, 5)

@ipdemes
Copy link
Contributor Author

ipdemes commented Apr 5, 2022

@manopapad : thank you for finding this bug. I will push the fix shortly

cunumeric/config.py Outdated Show resolved Hide resolved
In the case when input arrays (index arrays) are
broadcasted and, legate not always will partition them
(it will sometimes just broadcast them). This doesn't
effect correctness.
@ipdemes
Copy link
Contributor Author

ipdemes commented Apr 20, 2022

On the most recent checkout the index_routines.py test is failing on my local Mac, with:

I have fixed it by basically removing this check. In some cases, when index array is promoted at the front of the array, Legate will decide not to partition it and, instead, broadcast it to all index points. This doesn't change the logic in the task and doesn't effect correctness
@manopapad

cunumeric/deferred.py Outdated Show resolved Hide resolved
cunumeric/deferred.py Outdated Show resolved Hide resolved
cunumeric/deferred.py Outdated Show resolved Hide resolved
cunumeric/deferred.py Outdated Show resolved Hide resolved
@manopapad
Copy link
Contributor

This is ready to merge. Great work!

@ipdemes
Copy link
Contributor Author

ipdemes commented Apr 21, 2022

This is ready to merge. Great work!

Thank you for your help!

@ipdemes ipdemes merged commit bdbd205 into nv-legate:branch-22.05 Apr 21, 2022
@ipdemes ipdemes deleted the advanced_indexing branch May 26, 2022 20:20
ipdemes pushed a commit to ipdemes/cunumeric that referenced this pull request Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants