Add Gated Attention Reader

allenai / deep_qa

A deep NLP library, based on Keras / tf, focused on question answering (but useful for other NLP too)

Apache License 2.0

404 stars 132 forks source link

Add Gated Attention Reader #180

Closed nelson-liu closed 7 years ago

nelson-liu commented 7 years ago

This PR implements the Gated Attention Reader.

This still needs (roughly in order):

[x] a rebase on master, followed by fixes for the new encoder format (#175)
[x] tests
[x] calculation of the final question encoding from the cloze token index in the biGRU, not the entire output.
[x] pretrained wordvectors
[x] character level embeddings
[x] maybe add the question evidence common word feature?
[x] docs
[x] non-regression test for pretrained word embedding bug.

matt-gardner commented 7 years ago

Should I wait until all of the checkboxes are done before reviewing this?

nelson-liu commented 7 years ago

Yes, thanks!

nelson-liu commented 7 years ago

lastly this PR trains the GAReader on sciQ / validates on sciQ, and then runs evaluation on the SciQ test set.

nelson-liu commented 7 years ago

so i fixed the bug that made it impossible to use character embeddings + pretrained word vectors together due to conflicting layer names, but I'm finding it hard to add a non-regression test. Using an actual set of glove vectors would probably be way too slow, I guess perhaps we could write a "vector file" (and then gzip it, since the method takes gzipped files) like we do to test the models and read that?

matt-gardner commented 7 years ago

Yeah, gzipping the file in the test seems reasonable.

nelson-liu commented 7 years ago

So this PR as-is is ready for merging, but it's a bit abstruse and probably too large to review effectively. I've split it up into a bunch of mini PRs (#183, #184, #185, #186, #187). I'll leave this one open for archival purposes (/ a central branch to pull down if you actually want to run the code before it's all checked into master), and will close when its components are merged.

nelson-liu commented 7 years ago

superseded by #187 , closing.