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

adding support for array shape () passed as an index argument in advanced indexing #486

Merged
merged 9 commits into from
Aug 4, 2022

Conversation

ipdemes
Copy link
Contributor

@ipdemes ipdemes commented Aug 1, 2022

This PR making following work:

    arr[offsets[3]]=-1

@ipdemes ipdemes requested a review from manopapad August 1, 2022 15:57
@manopapad
Copy link
Contributor

manopapad commented Aug 2, 2022

This has different behavior from NumPy on get_item, because we are always creating a dim-1 output.

This code prints out [1]:

arr = 4 - cn.arange(5)
offsets = cn.arange(5)
offset = offsets[3]
print(arr[offset])

while NumPy returns 1:

arr = 4 - np.arange(5)
offsets = np.arange(5)
offset = offsets[3]
print(arr[offset])

I naively just tried removing the changes on deferred.py (and keeping just the C++ changes), but that triggers nv-legate/legate.core#314.

@ipdemes
Copy link
Contributor Author

ipdemes commented Aug 2, 2022

@manopapad : Thank you for finding this! I will wait for Wonchan to update on #314

@ipdemes
Copy link
Contributor Author

ipdemes commented Aug 2, 2022

@manopapad I have added fixes for few other cases when either lhs or rhs are futures

@ipdemes ipdemes requested a review from manopapad August 2, 2022 21:16
@ipdemes
Copy link
Contributor Author

ipdemes commented Aug 3, 2022

@manopapad @magnatelee : do you think we can still merge this to 22.07 since if fixes some corner cases of advanced indexing? Or should I change this PR to be merged to 22.10?

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

I added some further tests involving set/getitem on scalar arrays. It looks like there are issues when the index or value array is 0d and backed by a region-field:

arr = cn.full((5,), 42)
idx = cn.zeros((3,), dtype=np.int64)[2:3].reshape(())
arr[idx]

This example asserts inside of Legion:

[0 - 70000c362000]    2.048913 {5}{runtime}: [error 164] LEGION ERROR: Dynamic type mismatch in 'get_index_space_domain' (from file /Users/mpapadakis/legate.core/legion/runtime/legion/region_tree.inl:2843)

This might require changes in the core.

@ipdemes
Copy link
Contributor Author

ipdemes commented Aug 3, 2022

@manopapad I have reproduced the error and it seems it might take some time to fix it. Should we create a separate GitHub issue and PR for this use case?

@manopapad
Copy link
Contributor

Yes, let me update the testcase to skip the problematic cases, and I'll open a new issue about this.

@ipdemes
Copy link
Contributor Author

ipdemes commented Aug 3, 2022

@manopapad : do you think we can merge it when CI pass?

@manopapad manopapad changed the base branch from branch-22.07 to branch-22.10 August 3, 2022 21:15
@manopapad
Copy link
Contributor

@ipdemes I rebased the PR onto branch-22.10, and reformatted some testcases following guidance from @bryevdv. Let's verify that these pass before merging. Note that you will now need the new pytest-lazy-fixture package to run these tests.

@ipdemes ipdemes merged commit da3e661 into nv-legate:branch-22.10 Aug 4, 2022
@ipdemes ipdemes deleted the fix_scalar_args branch August 4, 2022 01:48
sbak5 pushed a commit to sbak5/cunumeric that referenced this pull request Aug 17, 2022
…nced indexing (nv-legate#486)

* fxing logic for some advanced_indexing test cases

* Reformatting of new testcases by @bryevdv

* Add new required test packages to conda env files

Co-authored-by: Manolis Papadakis <manopapad@gmail.com>
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.

2 participants