fix(torchlib): aten::unbind.int uses SplitToSequence(keepdims=False) to match PyTorch #2534
Conversation
…to match PyTorch shapes
|
Hi @microsoft/onnxscript- @maintainers I’ve submitted this PR to fix issue #2533 (
Kindly review this PR when you get a chance. Thanks a lot for your time and guidance |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2534 +/- ##
==========================================
+ Coverage 70.00% 70.02% +0.01%
==========================================
Files 215 215
Lines 25992 25992
Branches 2606 2606
==========================================
+ Hits 18196 18201 +5
+ Misses 6896 6891 -5
Partials 900 900 ☔ View full report in Codecov by Sentry. |
|
Hi @justinchuby @linshokaku, All local tests are passing ✅ |
| test("..", "..", "docs", "tutorial", "rewriter", "examples") | ||
|
|
||
|
|
||
| def test_unbind_matches_torch(): |
There was a problem hiding this comment.
I don't think this is the right place for the test. Please move to test_models_e2e and follow the format there
There was a problem hiding this comment.
All local lint/style/tests are passing ✅ and CI checks are currently running.
Please let me know if you’d prefer me to wait until all checks finish,
or if I should go ahead and make all the requested changes right away.
I’ll be happy to update accordingly. 🙂
There was a problem hiding this comment.
@justinchuby please reply what should i do?
There was a problem hiding this comment.
Hi @justinchuby, I noticed that test_models_e2e currently contains model-level tests like mobilenetv2_100 and resnet18.
Since my test validates correctness of a single op (unbind) against PyTorch rather than a model, could you please clarify where exactly I should move it?
- Should I create a new file under
unittest_models(since it’s op-level)? - Or should I still place it under
test_models_e2ein a new file dedicated for small op-level checks?
I want to make sure the test is placed in the right location and aligned with the project’s organization before I proceed with the changes. Thanks!
| Matches the behavior of torch.unbind. | ||
| """ | ||
| split_size = op.Constant(value_int=1) | ||
| return op.SplitToSequence(self, split_size, axis=dim, keepdims=False) |
There was a problem hiding this comment.
What excetly was updated?
There was a problem hiding this comment.
The update in core.py ensures that aten::unbind.int matches PyTorch’s torch.unbind behavior. Previously, the implementation returned tensors with an extra dimension (e.g., (3,1)), but with SplitToSequence(..., keepdims=False), it now correctly returns (3,) which aligns with PyTorch
|
If you are using an ai to assist with the change, please list the tools used. For now I think #2536 should fix it. Thanks! |
For transparency: I used GitHub Copilot/ChatGPT to assist in drafting the initial implementation and test. I manually reviewed, verified the changes, fixed style/lint issues, and ran the tests locally to ensure correctness before submitting @justinchuby |
|
Hi @MRADULTRIPATHI I think #2536 will fix the issue. I recommend heading over to https://github.com/microsoft/onnxscript/issues?q=sort%3Aupdated-desc%20is%3Aissue%20is%3Aopen%20label%3A%22contribution%20welcome%22&page=2 to see if there are other issues you find interesting. Thanks! |
|
Got it, thanks for clarifying! I’ll look into the open issues labeled "contribution welcome" and see where I can help contribute further. Appreciate the guidance @justinchuby Just to confirm — should I wait here for any further action, or go ahead and close this PR. |
Fixes #2533
What & Why
aten::unbind.intshould match PyTorch’storch.unbind, which removes the unbound axis.(3,1)instead of(3,), causing shape mismatches.Changes
aten::unbind.intusing opset18SplitToSequencewithkeepdims=False.aten::unbind.int.Tests
test_unbind_matches_torch) to compare shapes vs PyTorch.pytest docs/test/test_documentation_examples.py -v -k "test_unbind_matches_torch"