We should actually use /// instead of // because we need all the parameter comments to show up as docs in https://pytorch.org/cppdocs/api/structtorch_1_1nn_1_1_embedding_options.html#exhale-struct-structtorch-1-1nn-1-1-embedding-options.

Also I think we should change the comment to match the Python side:

ditto for matching Python side:

:attr: probably doesn't work in C++ API docs now, and we can change it to:

nit:

We might not need to specify c10::nullopt for padding_idx and max_norm, because the default value of c10::optional should already be c10::nullopt.

minor nit: we should likely swap the order here to match the Python implementation:

also options.weight_ can be changed to options._weight(), assuming we rename weight to _weight.

.contiguous() is not an in-place function, and we should likely do:

This also matches the Python implementation.

I also think we need to implement and call _no_grad_embedding_renorm_ here.

for _no_grad_embedding_renorm_, we can do this:

#include <torch/utils.h>
{
  torch::NoGradGuard no_grad;
  torch::embedding_renorm(...);
}

This should pass weight instead of *options.weight(), once we put weight back as the module's attribute.

We can use torch::nn::init::normal_(weight), if we include the torch/nn/init.h header.

keyword arguments don't work here because we are dealing with C++ :/ The expectation is that the user knows the first two arguments are num_embeddings and embedding_dim, and they can just call:

Since this one involves optional arguments, we would write:

ditto here, we would write:

ditto here, we would need to use EmbeddingOptions to express both the required arguments and optional arguments.

ditto here, we would need to use EmbeddingOptions to express both the required arguments and optional arguments.

ditto for TORCH_CHECK(condition, message)

ditto for TORCH_CHECK(condition, message)

same situation here, we should check } else if (*options.padding_idx() < 0) { instead, to match the Python implementation

We likely want torch::embedding_renorm_ instead of torch::embedding_renorm

A note on formatting: in general we would want to write

if (options.sparse()) {

instead of

if(options.sparse()){

, and

} else {

instead of

}
else {

to be consistent with the formatting of the other parts of C++ API.

Sorry for giving the wrong idea about how to implement from_pretrained: I think we will need to move from_pretrained to the Embedding class, not EmbeddingImpl, because we want people to be able to use it with torch::nn::Embedding::from_pretrained(...). I wrote a gist to illustrate the idea: https://gist.github.com/yf225/8eee0ef3f6afd2317092900927a43994.

We should also use EmbeddingOptions instead of writing out the parameters explicitly, as shown in the gist.

Also the return type shouldn't be a reference, and should just be Embedding (after we move this to the Embedding class).

ditto here: we would need to move from_pretrained to EmbeddingBag class, so that people can do torch::nn::EmbeddingBag::from_pretrained(...).

ditto for using EmbeddingBagOptions as well.

I think there are two issues here:

1. We should be checking the sizes of _weight instead of weight, to match the Python implementation.
2. In the error message, it should say Shape of weight instead of Shape of _weight, to match the Python implementation.

We can also improve the size checking with the following, to better match Python side:

We can simplify this as:

I think we might want to write the error message as following:

     "embedding_bag: If per_sample_weights (", per_sample_weights.sizes(), ") is not null, ",
     "then it must have the same shape as the input (", input.sizes(), ")");

Specifically there are a few issues with the original error message:

1. The spaces before then it must will be printed to the screen when the error message shows, which is likely not what we want.
2. We don't need \n at the end of the message because TORCH_CHECK will handle it automatically.
3. We can use .sizes() to simplify the size printing.

ditto here: we likely shouldn't put spaces before fixed length sequences, and should do

        "if input is 2D, then offsets has to be null, as input is treated is a mini-batch of ",
        "fixed length sequences ...");

We can probably just write However, found offsets of type Tensor, since this is enough information for the user to fix the issue.

We might want to do if (options._weight() == c10::nullopt) { to look more similar to Python implementation.

We need to pass the EmbeddingBag options to the constructor, because keyword argument is not supported in C++.

Also the return type shouldn't be a reference, and should just be Embedding (after we move this to the Embedding class).

We probably need to add tests for EmbeddingBag and Embedding::from_pretrained to the test suite. Some examples are as follows, copied from Python docs (https://pytorch.org/docs/stable/nn.html#embedding):

>>> # an Embedding module containing 10 tensors of size 3
>>> embedding_sum = nn.EmbeddingBag(10, 3, mode='sum')
>>> # a batch of 2 samples of 4 indices each
>>> input = torch.LongTensor([1,2,4,5,4,3,2,9])
>>> offsets = torch.LongTensor([0,4])
>>> embedding_sum(input, offsets)
tensor([[-0.8861, -5.4350, -0.0523],
        [ 1.1306, -2.5798, -1.0044]])

>>> # FloatTensor containing pretrained weights
>>> weight = torch.FloatTensor([[1, 2.3, 3], [4, 5.1, 6.3]])
>>> embeddingbag = nn.EmbeddingBag.from_pretrained(weight)
>>> # Get embeddings for index 1
>>> input = torch.LongTensor([[1, 0]])
>>> embeddingbag(input)
tensor([[ 2.5000,  3.7000,  4.6500]])

>>> # FloatTensor containing pretrained weights
>>> weight = torch.FloatTensor([[1, 2.3, 3], [4, 5.1, 6.3]])
>>> embedding = nn.Embedding.from_pretrained(weight)
>>> # Get embeddings for index 1
>>> input = torch.LongTensor([1])
>>> embedding(input)
tensor([[ 4.0000,  5.1000,  6.3000]])