-
Notifications
You must be signed in to change notification settings - Fork 88
[torchlib] Fix unbind.int if num_outputs=1 #2684
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
Conversation
|
If num_outputs=1, should we still use a Split node, or is Identity ok? |
Seems worthwhile emitting the optimized version without Split ... but it would still need the Squeeze ... no need for even Identity if we handle the two cases completely separately. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2684 +/- ##
==========================================
+ Coverage 70.11% 70.14% +0.02%
==========================================
Files 224 225 +1
Lines 26982 27042 +60
Branches 2705 2717 +12
==========================================
+ Hits 18919 18969 +50
- Misses 7129 7135 +6
- Partials 934 938 +4 ☔ View full report in Codecov by Sentry. |
Using Identity definitely makes sense, especially since this is already an optimization to my understanding. |
This fixes the issue of
when trying to export LSTM modules in
torch.This also already appeared in torch issues in pytorch/pytorch#126339
The core seems to be the changes in #2597.
To my understanding the split returns a single
SymbolicTensorinstead of a sequence whendim=1.The fix implemented here is the casting of the return type to a list.
I struggled with writing a test that reproduces this nicely in here, any guidance on that would be welcome.