[WIP v2 - deprecated] Unlikelihood token loss #2011
Conversation
funboarder13920
commented
Feb 19, 2021
funboarder13920
commented
- Implement unlikelihood loss from Neural Text Generation with Unlikelihood Training
- Tests for unlikelihood loss
- Class refactoring for LossComputeBase
- Fix ppl in statistics that used to be based on the loss which could be different from NLLLoss
funboarder13920
changed the title
.github/workflows/push.yml
Outdated
| -heads 4 -transformer_ff 64 \ | ||
| -word_vec_size 16 -report_every 5 \ | ||
| -rnn_size 16 -train_steps 10 | ||
| - name: Test LM training with unlieklihood loss |
| opt.label_smoothing, len(tgt_field.vocab), | ||
| ignore_index=padding_idx | ||
| ) | ||
| elif opt.unlikelihood_coeff > 0: |
There was a problem hiding this comment.
Shall we assert in the opts validation that unlikelihood_coeff isn't compatible with label_smoothing?
There was a problem hiding this comment.
I can make them mutually exclusive with the parser or I can make both of them compatible at the same time as unlikelihood_coeff can be added to any loss (but label smoothing and unlikelihood are contradictory)
There was a problem hiding this comment.
Yes, let's go for the mutually exclusive way.
| @@ -177,7 +177,7 @@ def forward(self, scores, align, target): | |||
| return loss | |||
|
|
|||
|
|
|||
| class CommonCopyGeneratorLossCompute(CommonLossCompute): | |||
| class CommonCopyGeneratorLossCompute(LossComputeBase): | |||
There was a problem hiding this comment.
I'm not sure to grasp the whole rationale behind the CommonLossCompute/LossComputeBase refactoring. Is the last big remaining difference only the log_ppl computation?
There was a problem hiding this comment.
(Underlying question is: do we really need both CommonLossCompute and LossComputeBase anymore?)
There was a problem hiding this comment.
The _compute_loss, _make_shard_state and the way to use the generator are different between CopyGeneratorLoss and the other classes
There was a problem hiding this comment.
We can do it in one class, the code is already not very clear, it's not going to be worse. If we do that CopyGenerator will override _compute_loss, _compute_log_ppl and _compute_alignement_loss will only be used in the compute_loss of the main class
There was a problem hiding this comment.
We can do it in one class, the code is already not very clear, it's not going to be worse. If we do that CopyGenerator will override _compute_loss, _compute_log_ppl and _compute_alignement_loss will only be used in the compute_loss of the main class
Yes I think this might be a bit better to explicitly override this method instead of having a full class that we don't really know what it's for unless we look at this specific CopyGeneratorLoss.
There was a problem hiding this comment.
I merged it, the ppl part is not nice. Also there is a normalization args that was not used anywhere, I will investigate to see if the normalization process disappeared by mistake
There was a problem hiding this comment.
normalization was already not used a year ago
Line 228 in 7835130
onmt/utils/loss.py
Outdated
| """ | ||
|
|
||
| def __init__(self, unlikelihood_coeff, ignore_index=-100): | ||
| assert 0.0 < unlikelihood_coeff |
There was a problem hiding this comment.
Maybe add an explicit message here?
onmt/utils/loss.py
Outdated
| target.size(0), target.size(1), target.size(0) | ||
| ).permute(1, 2, 0) | ||
|
|
||
| ctx_cands = ( |
There was a problem hiding this comment.
More explicit variable name? Or at least a comment?
|
@funboarder13920 @francoishernandez would it be worth updating wrt the v3 and merging or shall we drop ? |
vince62s
changed the title
