search

tensorflow / tfjs

A WebGL accelerated JavaScript library for training and deploying ML models.
https://js.tensorflow.org
Apache License 2.0
18.5k stars 1.93k forks source link

Add more tests for tf.fromPixels #1665

Closed tafsiri closed 2 years ago

tafsiri commented 5 years ago

TensorFlow.js version

Any

Describe the problem or feature request

There are some code paths in tf.fromPixels that are not tested. In particular we need to test passing in an HTMLCanvasElement, HTMLImageElement and HTMLVideoElement. The tests are located here.

In the past we had some issues testing these as interpolation made predicting the correct output values difficult. However we should test that no exceptions are thrown when we don't expect them to be, and the length of the data array to be what we expect.

Ultimately we want at least one test for every argument type that the function can accept.

WenheLI commented 5 years ago

Hi @tafsiri , just want to double check. I need to write three extra tests for HTMLCanvasElement,HTMLImageElementand HTMLVideoElement. Do I need to write for other types of parameter like OffscreenCanvas? Also, do I need to write them for both node environment and the browser environment? At last, I just need to check the output shape and no error is throwing during the run time for these unit tests. Am I understanding them correctly? Thanks

tafsiri commented 5 years ago

The first three tests are needed to make sure existing functionality doesn't break and should be added first (one PR). The test for OffscreenCanvas can be part of your existing PR that modifies fromPixels.

They only need to run in the browser environment BROWSER_ENVS (see the existing test for an example of how to constrain it to just that environment). I would check the output shape and also call .data() on the result and make sure the number of elements in the typedarray matches what you expect given the size of the element.

danwexler commented 2 years ago

I am using tf.fromPixels with an OffscreenCanvas, and it is working great. It would be fantastic if you could add the OffscreenCanvas type to the Typescript declaration for tf.fromPixels.

rthadur commented 2 years ago

@danwexler can you please open a new issue with ask , here is the reference code for tf.fromPixesl. Related PR has been merged and also I will close this as the related code has been moved to mono repo . Thank you