Add a sinusoidal embedding layer

[amantayal44]

Resolves #26

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

[amantayal44]

Hey @mattdangerw, any update on this?

[mattdangerw]

Thank you! Left some initial comments.

[amantayal44]

@mattdangerw, updated all the changes requested.

[mattdangerw]

Thanks! Few more comments.

[chenmoneygithub]

Thanks for the PR! The calculation looks good to me, left some initial comments.

[amantayal44]

@mattdangerw have small doubt when adding changes requested, should we do force push amending previous commit or create a new commit?

[mattdangerw]

@amantayal44 either is fine. We recently switched our merge strategy to "Squash and Merge", so you can keep a long commit history here of each round of review which will get squashed before going to master.

Also, re the argument name I saw you and Chen were discussing... I think the framing that will be most recognizable to users would be the exact framing from the Attention is All You Need paper, with the constant 10000 specifically. So switching this to wavelength instead of frequency. What do you think?

Most explainers I see out on the web use wavelength, and most implementations use 10K as a magic constant. Here's what the docstring could look like:

max_wavelength: The maximum angular wavelength of the sine/cosine curves, as 
    described in Attention is All You Need. Defaults to 10000.

def __init__(
    self,
    max_wavelength=10000,
    **kwargs,
)

Besides that, this lgtm!

[amantayal44]

@amantayal44 either is fine. We recently switched our merge strategy to "Squash and Merge", so you can keep a long commit history here of each round of review which will get squashed before going to master.

Also, re the argument name I saw you and Chen were discussing... I think the framing that will be most recognizable to users would be the exact framing from the Attention is All You Need paper, with the constant 10000 specifically. So switching this to wavelength instead of frequency. What do you think?

Most explainers I see out on the web use wavelength, and most implementations use 10K as a magic constant. Here's what the docstring could look like:

max_wavelength: The maximum angular wavelength of the sine/cosine curves, as 
    described in Attention is All You Need. Defaults to 10000.

def __init__(
    self,
    max_wavelength=10000,
    **kwargs,
)

Besides that, this lgtm!

yes, max_wavelength will be much better.

[mattdangerw]

@amantayal44 oops just noticed one more thing while merging. The filename no longer matches the class name.

Can you rename sine_positional_embedding.py -> sine_position_encoding.py, sine_positional_embedding_test.py -> sine_position_encoding_test.py, and fix the __init__.py import?

Then really good to go I think 😄

[amantayal44]

@amantayal44 oops just noticed one more thing while merging. The filename no longer matches the class name.

Can you rename sine_positional_embedding.py -> sine_position_encoding.py, sine_positional_embedding_test.py -> sine_position_encoding_test.py, and fix the __init__.py import?

Then really good to go I think 😄

done!! 😅

[mattdangerw]

Thank you!!