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

Added quantisation layer, commitment loss, and associated unit tests #29

Closed
wants to merge 7 commits into from

Conversation

RdoubleA
Copy link
Contributor

@RdoubleA RdoubleA commented Apr 26, 2022

Summary:

Vector Quantized Variational Autoencoder (VQVAE) is widely used to generate multimodal data (audio, image, or video samples). A well-known model implementing this is OpenAI's DALL-E model, which can generate images from text captions. The core component of the VQVAE is the quantisation layer, which is a discrete embedding of vectors that codifies the outputs of an encoder. Here, this layer is implemented as its own module, along with commitment loss which is used to fine-tune the encoder.

Test plan:

Unit tests were created and added in this PR. Testing was done with PyTest.

pytest -vv test/modules/losses/test_commitment.py

image

pytest -vv test/modules/layers/test_quantisation.py

image

@RdoubleA RdoubleA added the enhancement New feature or request label Apr 26, 2022
@RdoubleA RdoubleA self-assigned this Apr 26, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 26, 2022
Copy link
Contributor

@ankitade ankitade left a comment

Choose a reason for hiding this comment

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

overall good start. some questions / suggestions

torchmultimodal/modules/losses/vqvae.py Outdated Show resolved Hide resolved
torchmultimodal/modules/layers/quantisation.py Outdated Show resolved Hide resolved
torchmultimodal/modules/layers/quantisation.py Outdated Show resolved Hide resolved
torchmultimodal/modules/losses/vqvae.py Show resolved Hide resolved
test/modules/layers/test_quantisation.py Outdated Show resolved Hide resolved
test/modules/layers/test_quantisation.py Outdated Show resolved Hide resolved
torchmultimodal/utils/preprocess.py Outdated Show resolved Hide resolved
torchmultimodal/utils/preprocess.py Outdated Show resolved Hide resolved
torchmultimodal/modules/layers/quantisation.py Outdated Show resolved Hide resolved
torchmultimodal/modules/layers/quantisation.py Outdated Show resolved Hide resolved
test/modules/losses/test_commitment.py Outdated Show resolved Hide resolved
test/modules/losses/test_commitment.py Outdated Show resolved Hide resolved
test/modules/layers/test_quantisation.py Outdated Show resolved Hide resolved
torchmultimodal/utils/preprocess.py Outdated Show resolved Hide resolved
test/modules/layers/test_quantisation.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ebsmothers ebsmothers left a comment

Choose a reason for hiding this comment

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

Looks good! I like the way you structured the code, and adding the reshaping utils will come in handy as well.

torchmultimodal/modules/losses/vqvae.py Outdated Show resolved Hide resolved
torchmultimodal/utils/preprocess.py Outdated Show resolved Hide resolved
torchmultimodal/modules/layers/quantisation.py Outdated Show resolved Hide resolved
torchmultimodal/modules/layers/quantisation.py Outdated Show resolved Hide resolved
@langong347
Copy link
Contributor

Please include in the Test Plan:

  • the exact command you used to run unit tests regarding your changes
  • testing results

test/modules/layers/test_quantisation.py Outdated Show resolved Hide resolved
test/modules/layers/test_quantisation.py Outdated Show resolved Hide resolved
test/modules/layers/test_quantisation.py Outdated Show resolved Hide resolved
test/modules/layers/test_quantisation.py Outdated Show resolved Hide resolved
torchmultimodal/modules/layers/quantisation.py Outdated Show resolved Hide resolved
@langong347
Copy link
Contributor

Sorry I probably didn't make the purpose of Test Plan clear: You can just run tests related to your changes, e.g., pytest -vv test.modules.layers.test_quantisation.py something like this instead of executing all tests from the library.

@RdoubleA RdoubleA requested a review from langong347 April 28, 2022 23:42
Copy link
Contributor

@langong347 langong347 left a comment

Choose a reason for hiding this comment

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

Thanks for the nice work! Going through the threads in this PR I can see a few points to follow up:

  1. Learning of the codebook needs to be implemented by adding VQ loss or as moving average of the encoder output.
  2. Re-design of the commitment loss (or vqvae loss as a whole) and clean up API of the quantization layer.

Also I have a question for you:

  • The commitment loss is computed with the quantized vectors detached before returned by forward as in OpenAI and the Google example. In you current implementation, the quantized vectors are detached after (inside CommitmentLoss). Do you think the two steps are commutative?

Let me know your thoughts about those observation and if anything that might be missing.

Next step: click "import to fbsource" button below and wait for the internal CI to finish, fix any issues as needed.

@RdoubleA
Copy link
Contributor Author

Thanks for the nice work! Going through the threads in this PR I can see a few points to follow up:

  1. Learning of the codebook needs to be implemented by adding VQ loss or as moving average of the encoder output.
  2. Re-design of the commitment loss (or vqvae loss as a whole) and clean up API of the quantization layer.

Also I have a question for you:

  • The commitment loss is computed with the quantized vectors detached before returned by forward as in OpenAI and the Google example. In you current implementation, the quantized vectors are detached after (inside CommitmentLoss). Do you think the two steps are commutative?

Let me know your thoughts about those observation and if anything that might be missing.

Next step: click "import to fbsource" button below and wait for the internal CI to finish, fix any issues as needed.

I think as long as quantized is detached before computing loss then the gradients should be calculated correctly. Whether the loss is computed inside forward as in OpenAI's implementation or computed outside in a separate loss module as in CommitmentLoss the same tensors are being used, so it should have the same result.

@facebook-github-bot
Copy link
Contributor

@RdoubleA has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@RdoubleA has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ankitade ankitade deleted the vqvae branch December 7, 2022 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants