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 index_put with boolean index #2018

Merged
merged 22 commits into from
Jan 24, 2025
Merged

Conversation

xadupre
Copy link
Member

@xadupre xadupre commented Jan 17, 2025

Related: #1749

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.80%. Comparing base (e673351) to head (7aff13e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
onnxscript/function_libs/torch_lib/ops/core.py 92.30% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2018      +/-   ##
==========================================
+ Coverage   73.78%   73.80%   +0.02%     
==========================================
  Files         224      224              
  Lines       29264    29253      -11     
  Branches     3457     3455       -2     
==========================================
- Hits        21591    21589       -2     
+ Misses       6549     6536      -13     
- Partials     1124     1128       +4     

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

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

tests/function_libs/torch_lib/e2e_test.py:12

  • [nitpick] The function name '_index_put_failing_function' is ambiguous. Consider renaming it to '_index_put_with_boolean_index'.
def _index_put_failing_function(x_len, start_idx, left_window=0, right_window=0):

onnxscript/function_libs/torch_lib/ops/core.py:4301

  • Ensure that the new behavior introduced by the op.Where function is covered by tests to handle all cases correctly.
return op.Where(index, values, self)

onnxscript/function_libs/torch_lib/ops/core.py:4301

  • The comment about accumulate should be more explicit, or an assertion should be added to enforce that accumulate is False.
return op.Where(index, values, self)
@justinchuby justinchuby self-assigned this Jan 18, 2025
@justinchuby justinchuby added the module: torchlib Related to the torch/aten function lib in development label Jan 22, 2025
@justinchuby justinchuby enabled auto-merge (squash) January 24, 2025 01:48
@justinchuby justinchuby merged commit 6d2b530 into microsoft:main Jan 24, 2025
22 of 29 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