-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[OV JS] Add Tensor.copyTo() method to Node.js bindings #32340
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
base: master
Are you sure you want to change the base?
[OV JS] Add Tensor.copyTo() method to Node.js bindings #32340
Conversation
hi @almilosz @Retribution98 @mlukasze ,I have submitted this pull request to expose the Tensor.copyTo() method.please let me know your feedback on my PR , Thanks. Best Regards |
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.
Hello, I left some comments
if (info.Length() != 1) { | ||
reportError(env, "copyTo() must receive one argument, which is the destination Tensor."); | ||
return env.Undefined(); | ||
} | ||
|
||
if (!info[0].IsObject()) { | ||
reportError(env, "Invalid argument"); | ||
return env.Undefined(); | ||
} |
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.
Please use OPENVINO_ASSERT
. It's used in "newer" parts of js api
} | ||
|
||
try { | ||
auto dst_tensor_wrap = Napi::ObjectWrap<TensorWrap>::Unwrap(info[0].As<Napi::Object>()); |
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.
Please use
openvino/src/bindings/js/node/src/helper.cpp
Line 353 in 6e51b30
ov::Tensor cast_to_tensor(const Napi::CallbackInfo& info, int index) { |
|
||
} catch (const std::exception& e) { | ||
reportError(env, e.what()); | ||
return env.Undefined(); |
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.
return env.Undefined(); |
I think this one is not needed
}); | ||
|
||
describe("Tensor.copyTo()", () => { | ||
test("should copy data from source tensor to destination tensor with f32 type", () => { |
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.
Please parametrize tests for different data types
test("should throw error for incompatible element types", () => { | ||
const sourceData = new Float32Array([1.0, 2.0, 3.0, 4.0]); | ||
const sourceTensor = new ov.Tensor(ov.element.f32, [2, 2], sourceData); | ||
const destTensor = new ov.Tensor(ov.element.i32, [2, 2]); | ||
|
||
assert.throws(() => { | ||
sourceTensor.copyTo(destTensor); | ||
}); | ||
}); | ||
|
||
test("should copy when shapes differ but element count is same", () => { | ||
const sourceData = new Float32Array([1.0, 2.0, 3.0, 4.0]); | ||
const sourceTensor = new ov.Tensor(ov.element.f32, [2, 2], sourceData); | ||
const destTensor = new ov.Tensor(ov.element.f32, [4, 1]); | ||
|
||
assert.doesNotThrow(() => { | ||
sourceTensor.copyTo(destTensor); | ||
}); | ||
|
||
const destData = destTensor.getData(); | ||
assert.deepStrictEqual(Array.from(destData), Array.from(sourceData)); | ||
}); |
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.
Let's skip those, they are not directly related to binding.
test("should handle empty tensors", () => { | ||
const sourceTensor = new ov.Tensor(ov.element.f32, [0]); | ||
const destTensor = new ov.Tensor(ov.element.f32, [0]); | ||
|
||
assert.doesNotThrow(() => { | ||
sourceTensor.copyTo(destTensor); | ||
}); | ||
}); |
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.
test("should handle empty tensors", () => { | |
const sourceTensor = new ov.Tensor(ov.element.f32, [0]); | |
const destTensor = new ov.Tensor(ov.element.f32, [0]); | |
assert.doesNotThrow(() => { | |
sourceTensor.copyTo(destTensor); | |
}); | |
}); |
sourceTensor.data = modifiedSourceData; | ||
|
||
const destData = destTensor.getData(); | ||
assert.deepStrictEqual(Array.from(destData), [1.0, 2.0, 3.0, 4.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.
Nice!
Please also add assert that sourceTensor
contain [10.0, 20.0, 30.0, 40.0]
Details:
Implements ov::Tensor::copy_to() method in JavaScript bindings.