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 aten::diagonal #1755

Merged
merged 6 commits into from
Jul 31, 2024
Merged

[torchlib] Fix aten::diagonal #1755

merged 6 commits into from
Jul 31, 2024

Conversation

justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Jul 25, 2024

Turn aten::diagonal as trace only and fix its logic by explicitly converting python constants to onnx constants. This was needed because the exporter logic was not handling the type conversion correctly (yet)

@justinchuby justinchuby changed the title Fix aten::diagonal [torchlib] Fix aten::diagonal Jul 25, 2024
@justinchuby justinchuby added the topic: torch_lib Related to the torch/aten function lib in development label Jul 25, 2024
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.06%. Comparing base (a72f048) to head (27cf0e0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1755      +/-   ##
==========================================
- Coverage   75.07%   75.06%   -0.01%     
==========================================
  Files         245      245              
  Lines       26443    26437       -6     
  Branches     4824     4822       -2     
==========================================
- Hits        19852    19846       -6     
  Misses       5660     5660              
  Partials      931      931              

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

Copy link

github-actions bot commented Jul 25, 2024

Test Results

     24 files  ±0      24 suites  ±0   3h 14m 26s ⏱️ -8s
 15 569 tests  - 2  13 553 ✅  - 6    1 980 💤 ±0   36 ❌ +4 
459 690 runs   - 2  97 496 ✅  - 6  361 920 💤 ±0  274 ❌ +4 

For more details on these failures, see this check.

Results for commit 27cf0e0. ± Comparison against base commit a72f048.

This pull request removes 2 tests.
onnxscript.function_libs.tools.torch_lib.deduce_type_constraints_test.TestDeduceTypeConstraints ‑ test_deduce_type_constraints_does_not_crash_for_onnx_function__aten_diagonal_bool_onnx
onnxscript.function_libs.tools.torch_lib.deduce_type_constraints_test.TestDeduceTypeConstraints ‑ test_deduce_type_constraints_does_not_crash_for_onnx_function__aten_diagonal_onnx

♻️ This comment has been updated with latest results.

@justinchuby justinchuby marked this pull request as ready for review July 25, 2024 19:42
@fatcat-z
Copy link
Contributor

If possible, it'd better paste the error we saw into description so we know what kind of logic was fixed.

@justinchuby
Copy link
Collaborator Author

If possible, it'd better paste the error we saw into description so we know what kind of logic was fixed.

Done, thanks.

neg_1 = op.Constant(value_ints=[-1])
dim1_size = op.Reshape(op.Gather(op.Shape(self), dim1), neg_1) # row
dim2_size = op.Reshape(op.Gather(op.Shape(self), dim2), neg_1) # col
dim1_size = op.Shape(self, end=dim1, start=dim1 + 1) # row
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can dim1 be negative? Especially -1? Same for dim2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah it can. Thanks for pointing this out!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted to use gather. Added a comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm but its handled in
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In any case using Shape(start, end) does not work for some reason

@justinchuby justinchuby merged commit efe674d into main Jul 31, 2024
29 of 43 checks passed
@justinchuby justinchuby deleted the justinchu/variadic branch July 31, 2024 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: torch_lib Related to the torch/aten function lib in development
Projects
Development

Successfully merging this pull request may close these issues.

4 participants