-
Notifications
You must be signed in to change notification settings - Fork 422
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
Support signed value for Tensor::arange
#1238
Support signed value for Tensor::arange
#1238
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1238 +/- ##
==========================================
- Coverage 84.41% 84.38% -0.04%
==========================================
Files 549 550 +1
Lines 61952 62206 +254
==========================================
+ Hits 52295 52490 +195
- Misses 9657 9716 +59 ☔ View full report in Codecov by Sentry. |
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.
I saw some naming problems related to arange, I think we could fix this within this PR.
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.
-
A new unit test should be added to test all new edge cases (negatives and 0, overflow range).
-
Extra caution when casting usize to i64 because it overflow.
I think we should check if we can replace the usize by i64 instead of casting in most places. |
I moved the functions about arange to |
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.
LGTM
Pull Request Template
Checklist
run-checks all
script has been executed.Related Issues/PRs
#1234
Changes
Make
Tensor::arange
and all internal methods accept aRange<i64>
instead ofRange<usize>
to support ranges of signed value.Testing
bash ./run-checks.sh all
was passed.