Add a sinusoidal embedding layer

[mattdangerw]

Along with a learned positional embedding (#23), we should add a fixed sin/cos embedding as described in Attention is all you need.

[amantayal44]

I am familiar with transformer and TensorFlow, interested in taking up this issue.

[mattdangerw]

@amantayal44 that would be great!

@hertschuh is currently working on #23 which should be up for review shortly. It would probably be good to wait for us to at least discuss the overall design on that embedding layer before starting on this, so we can align on naming and the like. Then the code for that layer could be a good launching off point for this.

[mattdangerw]

OK, I think we have a clear enough path forward here. Let's name this SinusoidalPositionalEmbedding and include a max_length argument (though we may still tweak these). Mimic the inputs and outputs of #47, you should be able to swap one for the other.

A possible starting point for code for this is here.

@amantayal44 would you still like to work on this?

[amantayal44]

OK, I think we have a clear enough path forward here. Let's name this SinusoidalPositionalEmbedding and include a max_length argument (though we may still tweak these). Mimic the inputs and outputs of #47, you should be able to swap one for the other.

A possible starting point for code for this is here.

@amantayal44 would you still like to work on this?

yes sure.

I have few doubts

1. In TensorFlow implementation, there are using min_timescale and max_timescale, I am not clear about sinusoidal embedding using these two variables, In an original paper (https://arxiv.org/abs/1706.03762), there is only one variable, min_freq = 1e-4.

2. In sinusoidal encoding we don't need to know max_length beforehand, or should I pre-calculate the positional embedding matrix and avoid calculation at each call?

[mattdangerw]

Thank you!

re 1) good catch, let's stick to the original paper details when possible.

re 2) also good point, that's my bad. We can drop the max_length argument.

A couple other points from #23

• The learned position embedding layer will be assume the -2 axis is the sequence axis. We can do the same here for now (and possibly open this as a config option in the future).
• Also, the naming discussion continues over on this thread. Tentative new name SinePositionEncoding, we will see if that sticks 😁