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

Add concatenative feature embeddings #147

Merged
merged 8 commits into from Aug 5, 2017
Merged

Conversation

bpopeters
Copy link
Contributor

This pull request adds support for concatenation of word and feature embeddings. Namely, if you have an x-dimension word embedding and a y-dimensional feature embedding, then these will be concatenated into an (x+y)-dimensional embedding for the encoder. This is the default setting for using features in Lua.

I believe the previous behavior of the embeddings class corresponds to the sum behavior, so I preserved it with the sum option.

The goal is for the interface to be the same is in Lua OpenNMT. Consequently, the command line interface has been changed slightly to match Lua.

-feat_merge accepts concat and sum; the default is concat
-feat_vec_exponent defaults to 0.7; it is used to compute size of feature embeddings when using concat
-feature_vec_size is now -feat_vec_size and its default is 20

While I was at it, I changed the implementation of make_positional_encodings() to one with vectorized tensor operations instead of for loops and the built-in math class. It is faster now.

@srush
Copy link
Contributor

srush commented Aug 5, 2017

Great. This looks really good to me.

Also, I am moving this code around a bit in the torchtext branch, so I am also going to try to port it over as well. I might need you to take look there when it is done just to make sure everything got moved right.

@srush
Copy link
Contributor

srush commented Aug 5, 2017

Also are we sure that sum is the same? Is there a linear+relu in lua torch?

@srush srush merged commit 47305c7 into OpenNMT:master Aug 5, 2017
@bpopeters bpopeters deleted the embeddings branch August 5, 2017 09:07
@bpopeters
Copy link
Contributor Author

I'm not totally sure if the sum is the same; I looked in the lua and couldn't figure it out. But at the very least, the result is of the same dimension as the word_vec_size, which is what should happen in the sum case. I also couldn't find much explanation of what's supposed to go on internally with the sum.

@guillaumekln
Copy link
Contributor

Here is the Lua code:

https://github.com/OpenNMT/OpenNMT/blob/master/onmt/modules/FeaturesEmbedding.lua#L25

It just adds the embeddings, nothing more.

@bpopeters
Copy link
Contributor Author

Thanks. I interpreted what was going on primarily based on the documentation here:

http://opennmt.net/OpenNMT/options/train/#sequence-to-sequence-with-attention-options

The documentation seems to be in error with respect to feat_vec_size. In order to merge the feature and word embeddings (which is necessary if summing is the only thing going on), then there should be no reason to ever use feat_vec_size with sum: the only allowable feature size is the word_vec_size. I assumed that the documentation was correct here, which is why I didn't dismiss the mlp feature thing as the implementation of sum outright because it allows an arbitrary feat_vec_size and returns the same dimension as a summing feature merge.

I'll get cracking on fixing this soon.

@bpopeters bpopeters restored the embeddings branch August 8, 2017 08:02
marcotcr pushed a commit to marcotcr/OpenNMT-py that referenced this pull request Sep 20, 2017
Added producer-consumer pattern to MTurk polling
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.

None yet

3 participants