Implement cast #63
Implement cast #63
Conversation
test/cast_test.js
Outdated
| utils.checkValue(outputTensor, expected.data); | ||
| } | ||
|
|
||
| it('cast float32', function() { |
There was a problem hiding this comment.
Please rename test name as "cast float64 to float32", and same to others tests.
| case 'uint32': | ||
| outputArray = new Uint32Array(input.data); | ||
| break; | ||
| case 'int64': |
There was a problem hiding this comment.
Please fix this error "TypeError: Cannot convert a BigInt value to a number" of CI.
src/cast.js
Outdated
| case 'float16': | ||
| // todo | ||
| throw new Error('Unsupported output type: float16' ); | ||
| // case 'uint64': |
There was a problem hiding this comment.
There're two commented case block for uint64 type, please update it.
test/cast_test.js
Outdated
| const expected = { | ||
| shape: [5], | ||
| data: [ | ||
| 0, 0, 3, 210, 46, |
There was a problem hiding this comment.
210, 46
Casting between values that are beyond the limit of that data type is not well defined because different hardware will have different behaviors (CPU, NPU, GPU). Some will saturate 1234 to 255. Some will coerce to integer and apply bitwise truncation. This example clearly favors the typical x86 CPU behavior, but you will get surprises when you run this via DML, depending on which GPU is used.
(same comment applies elsewhere too for casts beyond bounds)
There was a problem hiding this comment.
Okay, thank! I'll convert examples outside the type range into within the range.
There was a problem hiding this comment.
+1, let's focus on the defined behavior at this stage.
test/cast_test.js
Outdated
| const input = { | ||
| shape: [5], | ||
| data: [ | ||
| -0.25, 0.25, 3.21, 1234, -1234, |
There was a problem hiding this comment.
Should include a case > X.5 too, like 3.7 for verify that truncation toward zero happens rather than rounding.
test/cast_test.js
Outdated
| const input = { | ||
| shape: [5], | ||
| data: [ | ||
| 0, 1, 2, 3, 3, |
There was a problem hiding this comment.
Including one negative number would be good.
test/utils.js
Outdated
| distance = distance >= 0 ? distance : -distance; | ||
| return assert.isTrue(distance <= nulp, message); | ||
| } else { | ||
| let distance = a-b; |
There was a problem hiding this comment.
[nit] a - b consistent spacing with line 33.
There was a problem hiding this comment.
(interesting - so in Javascript, int64 is considered not a "number" 🙃)
There was a problem hiding this comment.
The integer of type INT64 exceeds the range of JavaScript's Number type and belongs to the integer of BigInteger type. The JavaScript exception "can't convert BigInt to number" occurs when an arithmetic operation involves a mix of BigInt and Number values. FYI: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cant_convert_BigInt_to_number?retiredLocale=pl
src/cast.js
Outdated
| break; | ||
| case 'uint32': | ||
| outputArray = new Uint32Array(input.data); | ||
| outputArray = new Uint32Array(Array.from(input.data, (num) => (Math.round(num)))); |
There was a problem hiding this comment.
Truncation toward zero is the expected casting behavior for float to int, not rounding to nearest. That's consistent with existing ML libraries I know of. e.g. float32 3.7 -> uint32 3. Isn't the default behavior of Uint32Array already the expected truncation toward zero behavior? Otherwise we need Math.trunc().
There was a problem hiding this comment.
I'm sorry for the misunderstanding. I've revised the code :)
@BruceDai @huningxin @fdwr PTAL,thanks!