-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add embedding_renorm code | feat(torchlib) #1098
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1098 +/- ##
==========================================
+ Coverage 78.03% 78.10% +0.06%
==========================================
Files 115 115
Lines 14895 14932 +37
Branches 1581 1585 +4
==========================================
+ Hits 11623 11662 +39
+ Misses 2901 2900 -1
+ Partials 371 370 -1
|
Test Results 18 files ± 0 18 suites ±0 1h 17m 30s ⏱️ + 1m 0s For more details on these failures and errors, see this check. Results for commit 49fbcc6. ± Comparison against base commit 2a610c4. This pull request removes 147 and adds 153 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
unique_indices = op.Unique(indices) | ||
unique_indices_Y = op.SequenceAt(unique_indices, 0) | ||
# using _onnx private function because op.SrquenceAt(unique_indices, 0) cannot pass module checker | ||
# The error message is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we report this to ONNX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed an issue: onnx/onnx#5698
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎆 Thank you!
"""embedding_renorm(Tensor weight, Tensor indices, float max_norm, float norm_type) -> Tensor""" | ||
|
||
unique_indices = op.Unique(indices) | ||
unique_indices_Y = op.SequenceAt(unique_indices, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use slice here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop. It is an unequal length list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #1123. The output is not a Sequence. That’s why the checker complained
No description provided.