search

UppsalaNLP / uuparser

A transition-based parser for Universal Dependencies with BiLSTM word and character representations.
Apache License 2.0
80 stars 25 forks source link

Integrate ELMo into the UUParser #6

Closed jgontrum closed 5 years ago

jgontrum commented 5 years ago

Adds the ability to use ELMo embeddings as input features.

Breaking changes: The option --ext-emb-file, which was previous used for either external word- or character embeddings is replaced by --ext-word-emb-file and --ext-char-emb-file. This also fixes a bug, which did not allow the parser to learn character embeddings from scratch, while also using pre-trained word embeddings.

The exact changes for the fix can be examined here: https://github.com/jgontrum/uuparser-with-elmo/pull/2/files

jgontrum commented 5 years ago

Hey!

I wonder why you initalise elmo in src/arc_hybrid: it is not used there or am I missing something?

If I remember my reasoning correctly, I initialize the elmo object there to make it more visible what kind of data the feature extractor actually uses, as the arc_hybrid class is the driver of the whole process and should IMO be the place where all the objects are created. But we can also change that part of course.

Is there a reason the code for loading test embeddings is in src/arc_hybrid? It would be more appropriate that this code happens in src/feature_extractor, what do you think?

If you refer to the traditional embeddings, I just rewrote the part to work with both word- and character embeddings. I didn't want to do too much refactoring there.

mdelhoneux commented 5 years ago

I see your point but kind of disagree, in my opinion, elmo belongs to feature extraction and I think the ArcHybrid class, which is the main network, should be kept minimal with not much more than the parsing algorithm, the training and prediction procedures. Your second point is fair enough, I looked quickly and was no longer really familiar with that part of the code which was maintained by Aaron. I'm considering refactoring that but that has nothing to do with the pull request. I'll merge soon.