Skip to content

Accept multidimentional atomic indexes #1367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 27, 2024
Merged

Accept multidimentional atomic indexes #1367

merged 3 commits into from
Feb 27, 2024

Conversation

@ZzEeKkAa
Copy link
Contributor

@ZzEeKkAa ZzEeKkAa commented Feb 26, 2024

Previous implementation used to work only with flat indexes. This one accept multidimentional indexes.

Question: Should we remove default value for the index? It will cause TypeError on 2- and 3-dimentional arrays?

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • If this PR is a work in progress, are you filing the PR as a draft?

@ZzEeKkAa ZzEeKkAa self-assigned this Feb 26, 2024
@diptorupd
Copy link
Contributor

diptorupd commented Feb 26, 2024

Question: Should we remove default value for the index? It will cause TypeError on 2- and 3-dimentional arrays?

Yes, I think so. It was a bad design initially also. We should force people to be explicit with creation of atomic ref.

@ZzEeKkAa ZzEeKkAa force-pushed the feature/accept_multidimentional_atomic_indexes branch from 551749c to 7b4e818 Compare February 26, 2024 21:03
diptorupd
diptorupd previously approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@diptorupd diptorupd left a comment

Choose a reason for hiding this comment

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

LGTM! Good catch.

@diptorupd diptorupd self-requested a review February 27, 2024 03:32
@ZzEeKkAa ZzEeKkAa force-pushed the feature/accept_multidimentional_atomic_indexes branch from 867c370 to 13e6a9d Compare February 27, 2024 14:02
@ZzEeKkAa ZzEeKkAa enabled auto-merge February 27, 2024 14:02
@ZzEeKkAa ZzEeKkAa disabled auto-merge February 27, 2024 20:11
@ZzEeKkAa ZzEeKkAa merged commit 2df6fab into main Feb 27, 2024
@ZzEeKkAa ZzEeKkAa deleted the feature/accept_multidimentional_atomic_indexes branch February 27, 2024 20:12
github-actions bot added a commit that referenced this pull request Feb 27, 2024
@github-actions
Merge pull request #1367 from IntelPython/feature/accept_multidimenti…
15ceb15 
…onal_atomic_indexes

Accept multidimentional atomic indexes 2df6fab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adarshyoga adarshyoga Awaiting requested review from adarshyoga

1 more reviewer

@diptorupd diptorupd diptorupd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ZzEeKkAa ZzEeKkAa

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ZzEeKkAa @diptorupd