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

fix: out of bounds index access issue in argmin and argmax complex in CPU kernel #3176

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

ManasviGoyal
Copy link
Collaborator

No description provided.

@ManasviGoyal
Copy link
Collaborator Author

ManasviGoyal commented Jul 4, 2024

@jpivarski @ianna I added the unit tests tests for awkward_reduce_argmin_complex and awkward_reduce_argmin_complex and noticed that there was an out of bounds index access in the respective CPU kernels which gives a segmentation fault for the unit tests. Since these kernels were not tested earlier in the generated tests it might have been overlooked. There are integration tests already in tests directory that use these kernels which makes me suspect that they might be the reason for the pypy segfault (or was that fixed)?

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Good catch! This looks like it's more than fixing an out-of-bounds issue; the result wasn't right with the old indexing, was it? Are there any examples of tests that would fail numerically but not with an out of bounds index? (It's not necessary to produce one; I'm just asking.)

@ManasviGoyal
Copy link
Collaborator Author

Good catch! This looks like it's more than fixing an out-of-bounds issue; the result wasn't right with the old indexing, was it? Are there any examples of tests that would fail numerically but not with an out of bounds index? (It's not necessary to produce one; I'm just asking.)

For the already implemented integration tests and the unit tests I added, the results were actually correct numerically but for the unit tests it shows a segfault on running the tests in tests-cpu-kernels-explicit multiple times.

@ManasviGoyal ManasviGoyal marked this pull request as ready for review July 5, 2024 12:08
@ianna ianna self-requested a review July 19, 2024 12:16
@ManasviGoyal
Copy link
Collaborator Author

@ianna This one only changes the CPU kernels and since the CI tests pass, I think it can be merged. Thanks!

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ManasviGoyal - Good catch! Thanks! I'll merge it when the tests pass. Thanks!

@ianna ianna enabled auto-merge (squash) July 19, 2024 16:16
@ianna ianna disabled auto-merge July 19, 2024 16:44
@ianna ianna merged commit 4d5d49a into main Jul 19, 2024
40 checks passed
@ianna ianna deleted the ManasviGoyal/fix-argmin-argmax-complex-kernel branch July 19, 2024 16:49
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