fix: remove unconditional overwrite that disables teacher forcing in Seq2Seq - #1380
Conversation
|
There was a problem hiding this comment.
Pull request overview
Removes dead-code behavior in the thai2rom Seq2Seq implementation where teacher forcing was unintentionally disabled by an unconditional overwrite, aligning runtime behavior with the intended training/inference logic (fixes #1390).
Changes:
- Removed unconditional
decoder_input = topi.detach()that overwrote the teacher-forcing branch inSeq2Seq.forward().
| # Non-cryptographic use, pseudo-random generator is acceptable here | ||
| teacher_force = random.random() < teacher_forcing_ratio # noqa: S311 | ||
|
|
||
| if teacher_force and target_seq is not None: | ||
| decoder_input = target_seq[:, di].reshape(batch_size, 1) | ||
| else: | ||
| decoder_input = topi.detach() |
There was a problem hiding this comment.
The teacher-forcing behavior was previously broken by an unconditional overwrite; this change restores it, but there isn’t a regression test ensuring teacher_forcing_ratio actually affects decoder_input selection. Please add a unit test that forces the teacher-forcing branch deterministically (e.g., by patching random.random) and asserts decoder_input comes from target_seq when enabled, so this doesn’t regress again.
There was a problem hiding this comment.
Note that since thai2rom uses torch, it's tests are in "noauto" test group.
The noauto group will not be run on the CI. Have to run the tests locally.
|
This PR is stale because it has been open for 30 days with no activity. |
What do these changes do
Remove unconditional
decoder_input = topi.detach()that overwrites the teacher forcing if-else blockFixes #1390