Fixes for seq2seq model #1808
Fixes for seq2seq model #1808
Conversation
- Addresses:
- possible masking bug in simple_seq2seq #1713, "possible masking bug in simple_seq2seq"
- Simple Seq2Seq Computes Incorrect Dev Log Loss #1134, "Simple Seq2Seq Computes Incorrect Dev Log Loss"
|
(Needs an extra test before review.) |
|
Thanks! I added a simple test for an issue that using Let me know if it's worth having a test specific to the masking issue. |
There was a problem hiding this comment.
(Sorry, I think I started looking over the code before you unrequested your review, and approved it before seeing that you wanted me to wait. It still looks good =).)
I don't think we need any more specific test for the masking issue. It would be way more work than it's worth to make that actually testable in this model code, and the function you're calling already has its own unit tests.
And no, we don't have any tests that do larger tests of training models on real data. That might be nice to have, to run occasionally (nightly?), but we definitely don't want something that big in our PR CI.
| def test_encoder_decoder_can_train_save_and_load(self): | ||
| self.ensure_model_can_train_save_and_load( | ||
| self.param_file, | ||
| overrides="{model: {encoder: {bidirectional: true}}}" |
There was a problem hiding this comment.
Why not just change this in experiment.json?
There was a problem hiding this comment.
I'd prefer to test with bidirectional set to both true and false. And, naturally, duplicating the config file wasn't very compelling.
There was a problem hiding this comment.
Oh, ok. It looks like you can just add this as another method on the other class, too, instead of making a whole new class for this.
|
(No worries!) Thanks for the background on the tests. I filed #1813 so we can consider the periodic retraining. |