Skip to content
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

adds support for Uint8ClampedArray #21985

Merged
merged 22 commits into from
Sep 12, 2024
Merged

Conversation

prathikr
Copy link
Contributor

@prathikr prathikr commented Sep 4, 2024

Fixes #21753

@prathikr prathikr requested a review from fs-eire September 4, 2024 23:34
@fs-eire
Copy link
Contributor

fs-eire commented Sep 4, 2024

also need to add type declaration for uint8 in common\lib\tensor.ts

@fs-eire
Copy link
Contributor

fs-eire commented Sep 4, 2024

can also add one test in js/common/unit-test/tensor/constructor-type.ts for:

[uint8] new Tensor(uint8ClampedArray, dims): uint8 tensor can be constructed from Uint8ClampedArray
[uint8] new Tensor('uint8', uint8ClampedArray, dims): uint8 tensor can be constructed from Uint8ClampedArray

js/common/lib/tensor.ts Outdated Show resolved Hide resolved
@prathikr prathikr requested a review from fs-eire September 5, 2024 21:59
@fs-eire
Copy link
Contributor

fs-eire commented Sep 6, 2024

seems still need code formatting

js/common/lib/tensor.ts Outdated Show resolved Hide resolved
@prathikr prathikr requested a review from fs-eire September 9, 2024 18:48
@fs-eire
Copy link
Contributor

fs-eire commented Sep 10, 2024

To precisely explain the requirement of "support ClamppedUint8Array", it means "to allow creating uint8 tensors from ClamppedUint8Array". It does not change the underlying storage of a uint8 tensor (it is always Uint8Array). To break down, the work items are:

  • update TypeScript API to add new Tensor(data: ClamppedUint8Array, dims?: readonly number[])
  • update TypeScript API to add new Tensor(type: 'uint8', data: ClamppedUint8Array, dims?: readonly number[])
  • update actual implementation to enable the 2 interfaces mentioned above
  • add TypeScript typecheck and constructor unittest

Allowing users to create a uint8 tensor from both Uint8Array and ClamppedUint8Array does not mean there should be 2 different types ('uint8' and 'uint8c'). Tensor types are logically mapping to ONNX Tensor data type, and there is no point to introduce 2 types for uint8.

@prathikr
Copy link
Contributor Author

@fs-eire I reverted the change to previous solution which has Uint8ClampedArray implemented as a uint8 datatype. However, I cannot seem to find a way to do this without including uint8: Uint8Array | Uint8ClampedArray; in DataTypeMap which you previously mentioned will affect type inferencing.

Please advise on if this is ok or what I can do differently. The current code passes the tests I've set up.

@prathikr prathikr requested a review from fs-eire September 11, 2024 06:37
@fs-eire
Copy link
Contributor

fs-eire commented Sep 11, 2024

test is failed.

@prathikr prathikr merged commit d495e6c into main Sep 12, 2024
95 of 96 checks passed
@prathikr prathikr deleted the prathikrao/js-Uint8ClampedArray-support branch September 12, 2024 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Web] Allow passing Uint8ClampedArray to ort.Tensor()
2 participants