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 bounds check for advanced indexing #397

Merged
merged 6 commits into from
Jun 24, 2022

Conversation

ipdemes
Copy link
Contributor

@ipdemes ipdemes commented Jun 13, 2022

addressing issue #31

@ipdemes ipdemes requested a review from magnatelee June 13, 2022 11:28
@magnatelee
Copy link
Contributor

@ipdemes is this a complete PR? I don't see any changes on the Python side to make the exceptions re-raised.

@ipdemes
Copy link
Contributor Author

ipdemes commented Jun 14, 2022

@ipdemes is this a complete PR? I don't see any changes on the Python side to make the exceptions re-raised.

That is right, I didn't push changes to the python code

@manopapad
Copy link
Contributor

I think we should throw the standard Python IndexError exception here, as NumPy does. We don't need a custom CuNumericError in this case.

>>> import numpy as np
>>> x = np.ones((4,))
>>> x[[0,1,2,3,4]]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: index 4 is out of bounds for axis 0 with size 4

@ipdemes
Copy link
Contributor Author

ipdemes commented Jun 14, 2022

@manopapad makes sense. Will change this

cunumeric/exception.py Outdated Show resolved Hide resolved
@ipdemes ipdemes requested a review from manopapad June 15, 2022 08:30
@@ -45,6 +45,9 @@ class ZipTask : public CuNumericTask<ZipTask> {

constexpr coord_t compute_idx(coord_t index, coord_t dim)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the variable name dim is very confusing. I believe you meant extent, right? dim seems synonymous with axis in other contexts, so let's change this.

@@ -45,6 +45,9 @@ class ZipTask : public CuNumericTask<ZipTask> {

constexpr coord_t compute_idx(coord_t index, coord_t dim)
{
// bounds checking
if ((index < 0 && index + dim < 0) || (index >= 0 && index >= dim))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually rewrite this function to something like this:

coord_t sanitized_index = index < 0 ? index + dim : index;
if (sanitized_index < 0 || sanitized_index >= extent) throw ...
return sanitized_index;

@magnatelee
Copy link
Contributor

@ipdemes merge this with the latest branch-22.07, then you won't see the failure on 2 GPUs anymore.

@ipdemes ipdemes requested a review from magnatelee June 22, 2022 19:40
@ipdemes
Copy link
Contributor Author

ipdemes commented Jun 23, 2022

@ipdemes merge this with the latest branch-22.07, then you won't see the failure on 2 GPUs anymore.

done

@ipdemes ipdemes merged commit 8a44933 into nv-legate:branch-22.07 Jun 24, 2022
@ipdemes ipdemes deleted the ai_bounds_check branch August 2, 2022 17:33
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