Pytorch embedding #2
Conversation
|
|
||
| Parameters | ||
| ---------- | ||
| num_embeddings:, int: |
There was a problem hiding this comment.
We should probably figure out markdown vs. rest sooner rather than later, so we can switch docstrings now, if we're going to switch.
If we're sticking with rest, this should be formatted like:
num_embeddings : ``int``
Note the space after the parameter name - it's important for the code that generates the docs.
| self.weight.data[self.padding_index].fill_(0) | ||
|
|
||
| def forward(self, input): | ||
| padding_idx = self.padding_idx |
| self.sparse = sparse | ||
|
|
||
| if embedding_dim == 1: | ||
| raise ConfigurationError("There is no need to embed tokens if you " |
There was a problem hiding this comment.
Why not? I could be learning something like a tf-idf weight for each token. Seems odd to put in this restriction, as it's unnecessary.
There was a problem hiding this comment.
Sure, I was just porting the previous embedding. I'll remove it.
| embedding_dim: int, | ||
| weight: torch.FloatTensor=None, | ||
| padding_index: int=None, | ||
| trainable: bool=True, |
There was a problem hiding this comment.
This field is missing from the docstring.
| if weight is None: | ||
| weight = torch.FloatTensor(num_embeddings, embedding_dim) | ||
| self.weight = torch.nn.Parameter(weight, requires_grad=trainable) | ||
| self.reset_parameters() |
There was a problem hiding this comment.
How does this work? This is scary to me.
There was a problem hiding this comment.
Oh, you defined this method below. Ok, that makes more sense. When I first saw this, I thought you were calling a super class method, and I was really nervous about using pytorch.
There was a problem hiding this comment.
Do you even need this method? Looks like it's just a single line, and you can move the padding fill to below the if/else block, because you do it in both.
There was a problem hiding this comment.
And if you do want it, I'd call it something like initialize_parameters.
There was a problem hiding this comment.
It looks like there is a technical reason to have this function if we ever need to call it to create parameters in the call of forward: https://discuss.pytorch.org/t/dynamic-parameter-declaration-in-forward-function/427/7
This isn't the reason it's used here though, but I was wondering why other pytorch layers use it and it looks like this might be why.
There was a problem hiding this comment.
Interesting, ok. But it's not a method defined on Module, so we can handle this however we want, right?
There was a problem hiding this comment.
Yes, I have removed it for now.
| trainable: bool=True, | ||
| log_misses: bool=False): | ||
| """ | ||
| Reads a pre-trained embedding file and generates a Keras Embedding layer that has weights |
| it a zero vector. | ||
|
|
||
| The embeddings file is assumed to be gzipped, formatted as [word] [dim 1] [dim 2] ... | ||
| """ |
There was a problem hiding this comment.
Documenting parameters here would be good.
| embedding_dim = None | ||
|
|
||
| # TODO(matt): make this a parameter | ||
| embedding_misses_filename = 'embedding_misses.txt' |
There was a problem hiding this comment.
We can probably just log misses using logger.debug, and remove the log_misses parameter entirely, and this variable.
| assert embedding_dim > 1, "Found embedding size of 1; do you have a header?" | ||
| else: | ||
| if len(fields) - 1 != embedding_dim: | ||
| # Sometimes there are funny unicode parsing problems that lead to different |
There was a problem hiding this comment.
Can we do better here? Maybe we can just import a library that reads glove vectors already, so we don't have to duplicate that functionality?
|
|
||
| # Depending on whether the namespace has PAD and UNK tokens, we start at different indices, | ||
| # because you shouldn't have pretrained embeddings for PAD or UNK. | ||
| start_index = 0 if namespace.startswith("*") else 2 |
There was a problem hiding this comment.
You should be able to always start at 0. The check for word in embeddings will fail, and everything will be ok, because we just continue in that case. And the namespace check is wrong here, anyways.
otherwise it would raise an error in line 505 (expected object of type torch.FloatTensor but found type torch.cuda.FloatTensor for argument #2 'other')
otherwise it would raise an error in line 505 (expected object of type torch.FloatTensor but found type torch.cuda.FloatTensor for argument #2 'other')
otherwise it would raise an error in line 505 (expected object of type torch.FloatTensor but found type torch.cuda.FloatTensor for argument allenai#2 'other')
segmental conll 2000 dataset reader
Pull from AllenNLP Master
Switching the word embedding functionality over to use Pytorch. Mainly did this to get a quick idea of how tough it would be to switch layers. Answer: pretty straightforward.