Skip to content

Update webcam-transfer-learning with tf.data.webcam API #267

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

Merged
merged 8 commits into from
Jun 12, 2019
Merged

Conversation

kangyizhang
Copy link

@kangyizhang kangyizhang commented Apr 17, 2019

This change is Reviewable

@bileschi
Copy link
Contributor


webcam-transfer-learning/package.json, line 13 at r1 (raw file):

@tensorflow/tfjs-data": "link:.yalc/@tensorflow/tfjs-data",

Probably didn't mean to include this?

Copy link
Contributor

@bileschi bileschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang)

@kangyizhang kangyizhang changed the title [DO NOT REVIEW] use tf.data.webcam for webcam-transfer-learning Update webcam-transfer-learning with tf.data.webcam API May 16, 2019
@kangyizhang kangyizhang requested review from caisq and dsmilkov May 16, 2019 18:47
Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @caisq, @dsmilkov, and @kangyizhang)


webcam-transfer-learning/index.js, line 160 at r2 (raw file):

return (await webcam.capture())

The intermediate tensors (i.e., the one output by expanDims(0)) should be tidied. Otherwise, you'll get memory leaks.


webcam-transfer-learning/index.js, line 163 at r2 (raw file):

tf.scalar(127)

This can be simplified to div(127). Same for the sub() call below.

Copy link
Author

@kangyizhang kangyizhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bileschi, @caisq, and @dsmilkov)


webcam-transfer-learning/index.js, line 160 at r2 (raw file):

Previously, caisq (Shanqing Cai) wrote…
return (await webcam.capture())

The intermediate tensors (i.e., the one output by expanDims(0)) should be tidied. Otherwise, you'll get memory leaks.

The returned tensor is disposed at line: https://github.com/tensorflow/tfjs-examples/pull/267/files#diff-030148133b399b3f844d841a0514bfbbR58

It seems that tf.tidy() can not be used here, because webcamIterator.capture() is async function, and async functions are not allowed in tf.tidy().


webcam-transfer-learning/index.js, line 163 at r2 (raw file):

Previously, caisq (Shanqing Cai) wrote…
tf.scalar(127)

This can be simplified to div(127). Same for the sub() call below.

Done.


webcam-transfer-learning/package.json, line 13 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
@tensorflow/tfjs-data": "link:.yalc/@tensorflow/tfjs-data",

Probably didn't mean to include this?

removed.

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bileschi, @caisq, @dsmilkov, and @kangyizhang)


webcam-transfer-learning/index.js, line 160 at r2 (raw file):

Previously, kangyizhang (Kangyi Zhang) wrote…

The returned tensor is disposed at line: https://github.com/tensorflow/tfjs-examples/pull/267/files#diff-030148133b399b3f844d841a0514bfbbR58

It seems that tf.tidy() can not be used here, because webcamIterator.capture() is async function, and async functions are not allowed in tf.tidy().

I understand you dispose the tensor returned by this function. However, this function allocates other tensors that are not cleaned up, which leads to memory leak. You can do something like the following:

const img = await webcam.capture();
const processedImg = tf.tidy(() => img.expandDims(0).toFloat().div(127).sub(1));
img.dispose();
return processedImg;

Copy link
Author

@kangyizhang kangyizhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bileschi, @caisq, and @dsmilkov)


webcam-transfer-learning/index.js, line 160 at r2 (raw file):

Previously, caisq (Shanqing Cai) wrote…

I understand you dispose the tensor returned by this function. However, this function allocates other tensors that are not cleaned up, which leads to memory leak. You can do something like the following:

const img = await webcam.capture();
const processedImg = tf.tidy(() => img.expandDims(0).toFloat().div(127).sub(1));
img.dispose();
return processedImg;

Done. Sorry I lost track of this PR and thank you for explaining this to me.

@kangyizhang
Copy link
Author

Friendly ping.

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bileschi, @caisq, and @dsmilkov)

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @bileschi, @caisq, and @dsmilkov)

@caisq caisq merged commit fc8646f into master Jun 12, 2019
@mihaimaruseac mihaimaruseac deleted the webcam branch December 16, 2020 22:10
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.

3 participants