search

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 133 forks source link

Got dynamic padding actually working for sequence tagger #319

Closed matt-gardner closed 7 years ago

matt-gardner commented 7 years ago

The main issue was with the CNN encoder used to get word encodings. I needed to make it agnostic to the input length, which it now is.

This led to a ~4x speedup in runtime, going from over 4000s per epoch to ~1000s. And that's an automatic gain, with a couple of flags, for any model that's compatible. I'm pretty happy about this. @matt-peters, thanks for the idea! The only issue is that some of our more complex models will be hard to make compatible with dynamic padding...

matt-gardner commented 7 years ago

@DeNeutoy, to address a comment that (I think) you deleted, you're right. This is similar to the sorting_keys stuff I added a little while ago - I originally had that as a field on TextTrainer that subclasses would override, but decided it was easier to document and resulted in a cleaner API to make it a method instead. I think that's appropriate to do here, as well, just so we're consistent about what kinds of things you have to override in subclasses. It is indeed really complicated if you have to be careful to pop a parameter before calling the superclass, then set a field after calling the superclass. Too much magic. I'll switch it to a method, so parameter passing will be more sane, and so that it shows up in the docs.

matt-gardner commented 7 years ago

@DeNeutoy, here's another try. Does this look better?

DeNeutoy commented 7 years ago

Much better ☺️