Multi input time distributed - Githubissues

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

Multi input time distributed #207

Closed BeckySharp closed 7 years ago

BeckySharp commented 7 years ago

I'd love you to:

check that the tests for the call method are sufficient to really make sure things are ok
check the compute_mask method -- I tried to borrow from the TD with Mask class and extend it parallel to how Mark helped me extend the call method of the multi Input one
help me write a test for the compute_mask (right now, it's a bunch of copied code from other tests that I half modified before quitting)
make sure I didn't ruin things with the move to wrappers/ with separate files...

matt-gardner commented 7 years ago

Thanks for working on this, it's a really helpful change.

matt-gardner commented 7 years ago

I'm going to try to finish this now, because it'd be easier for me than for you, and so you can get back to doing model design sooner. If I finish this fast enough, I'll work on the suggestions I made to your other PR, too. You've gotten them most of the way there, so it shouldn't take me too long to finish it.

BeckySharp commented 7 years ago

sorry I didn't do this yet -- just wasn't working on ai2 stuff today

On Sat, Feb 18, 2017 at 8:50 PM, Matt Gardner notifications@github.com wrote:

I'm going to try to finish this now, because it'd be easier for me than for you, and so you can get back to doing model design sooner. If I finish this fast enough, I'll work on the suggestions I made to your other PR, too. You've gotten them most of the way there, so it shouldn't take me too long to finish it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/allenai/deep_qa/pull/207#issuecomment-280896073, or mute the thread https://github.com/notifications/unsubscribe-auth/AFInifTthoQaYZl8J7vcMzRkYqK0l4cMks5rd8n6gaJpZM4MEqEw .

matt-gardner commented 7 years ago

No need to apologize for not working on a Saturday. It's not expected at all. I'm just trying to help you get back to stuff that's more interesting for you. Improving the library probably isn't the most exciting thing for you to be working on.

matt-gardner commented 7 years ago

Ok, I think this is done. It turned out it was more complicated than I originally thought it was (which makes me happy that I could do it for you, because it would have taken quite a bit of back-and-forth to figure out the issues and the solutions to them, and that would have just been a distraction for your internship project). But it should work now; you should be able to just use our TimeDistributed class on multiple inputs, with masks, and it should just work.

matt-gardner commented 7 years ago

And I'm going to add you as a reviewer to the PR - can you look over the changes I made? Oh, I can't add you as a reviewer, because you submitted the PR... But you can still look it over =).

BeckySharp commented 7 years ago

thanks again for all your help on this! looks great to me, and seems like you cleaned it all up by combining use cases, but then again, take my input with the appropriately sized grain of salt!