Skip to content

[torchlib] Fix index_put: handle None cases#2061

Merged
justinchuby merged 4 commits intomicrosoft:mainfrom
AyoubMDL:index_input_fix
Mar 4, 2025
Merged

[torchlib] Fix index_put: handle None cases#2061
justinchuby merged 4 commits intomicrosoft:mainfrom
AyoubMDL:index_input_fix

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

@codecov
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
@github-project-automation github-project-automation bot moved this from Todo to Done in ONNX Script Review Board Mar 4, 2025
@AyoubMDL
Copy link
Contributor Author

AyoubMDL commented Mar 5, 2025

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

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