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

[torchlib] Fix index_put: handle None cases #2061

Merged
merged 4 commits into from
Mar 4, 2025

Conversation

AyoubMDL
Copy link
Contributor

@AyoubMDL AyoubMDL commented Feb 17, 2025

This PR introduces support for None indices in the index_put function. If an index is None, it acts as a full slice (:).

index_put Logic:

  1. Construct index grid that contains all the indices to be updated
  2. Reshapes the update values to match the computed indices.

@AyoubMDL
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Brainchip"

@AyoubMDL AyoubMDL marked this pull request as ready for review February 18, 2025 17:15
@AyoubMDL
Copy link
Contributor Author

@justinchuby PR is ready for review

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.24%. Comparing base (89dd454) to head (959dab4).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2061      +/-   ##
==========================================
- Coverage   72.35%   72.24%   -0.11%     
==========================================
  Files         216      217       +1     
  Lines       28890    29053     +163     
  Branches     3433     3449      +16     
==========================================
+ Hits        20904    20990      +86     
- Misses       6856     6934      +78     
+ Partials     1130     1129       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justinchuby
Copy link
Collaborator

justinchuby commented Feb 18, 2025

Thank you! Could you look at the failing tests?

@justinchuby justinchuby changed the title fix(index_put): handle None cases [torchlib] Fix index_put: handle None cases Feb 18, 2025
@justinchuby justinchuby added the module: torchlib Related to the torch/aten function lib in development label Feb 18, 2025
@justinchuby justinchuby self-requested a review February 18, 2025 18:12
@titaiwangms titaiwangms self-requested a review February 18, 2025 19:21
@AyoubMDL
Copy link
Contributor Author

Force pushed as I changed a little bit the logic. Also added more test cases.

@justinchuby justinchuby added this to the 0.2.1 milestone Feb 21, 2025
@justinchuby justinchuby self-assigned this Feb 21, 2025
@justinchuby justinchuby modified the milestones: 0.2.1, 0.3 Feb 27, 2025
@justinchuby
Copy link
Collaborator

Could you take a look at the falling CI, and we can merge this PR? Thanks!

@AyoubMDL
Copy link
Contributor Author

AyoubMDL commented Feb 27, 2025

Could you take a look at the falling CI, and we can merge this PR? Thanks!

it was missing a case with multidimensional index (added a test for that case). Tell me if other cases are missing that you want to support.

@justinchuby
Copy link
Collaborator

For range, I think we need to Squeeze the tensor to remove its dimension.

@AyoubMDL AyoubMDL marked this pull request as draft February 28, 2025 16:49
@AyoubMDL
Copy link
Contributor Author

AyoubMDL commented Mar 4, 2025

@justinchuby I wasn’t able to make this work on symbolic tensors. Please feel free to close the draft or continue from here if helpful.

@justinchuby
Copy link
Collaborator

Thanks. Could you share the issues you have seen? I can take it from here

@justinchuby justinchuby marked this pull request as ready for review March 4, 2025 18:34
@justinchuby justinchuby enabled auto-merge (squash) March 4, 2025 18:34
@justinchuby justinchuby disabled auto-merge March 4, 2025 18:34
@justinchuby
Copy link
Collaborator

Will merge as-is and create follow up improvements

@justinchuby justinchuby merged commit 8e53070 into microsoft:main Mar 4, 2025
23 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: torchlib Related to the torch/aten function lib in development
Projects
Development

Successfully merging this pull request may close these issues.

3 participants