speechbrain icon indicating copy to clipboard operation
speechbrain copied to clipboard

[Bug]: speechbrain.decoders.seq2seq.py

Open zhhuang93 opened this issue 2 years ago • 3 comments

Describe the bug

Is there any bugs in coverage penalty? In Line 750, should "self.converage" be "self.coverage"? and the process will always go into len(cur_attn.size()) > 2, but not conduct self.coverage = self.coverage + cur_attn, does it make sense?

Expected behaviour

Just refer to paper https://arxiv.org/pdf/1612.02695.pdf, I am wondering if the implementation is inconsistent with the formula in the paper? Thanks!

To Reproduce

No response

Versions

No response

Relevant log output

No response

Additional context

No response

zhhuang93 avatar Jan 27 '23 06:01 zhhuang93

Hi, this is a typo. We fixed it in a refactored version but have not merged it yep (Ping @mravanelli). https://github.com/speechbrain/speechbrain/blob/a7c4e44c3176a699cbaac3cd90afc66817b9f7d3/speechbrain/decoders/scorer.py#L569-L574

Regarding len(cur_attn.size()) > 2, are you running the transformer recipe? If so the coverage is the sum of attn from the transformer.

I am wondering if the implementation is inconsistent with the formula in the paper?

Could you specify which part you find it inconsistent?

30stomercury avatar Jan 27 '23 11:01 30stomercury

Hi, this is a typo. We fixed it in a refactored version but have not merged it yep (Ping @mravanelli).

https://github.com/speechbrain/speechbrain/blob/a7c4e44c3176a699cbaac3cd90afc66817b9f7d3/speechbrain/decoders/scorer.py#L569-L574

Regarding len(cur_attn.size()) > 2, are you running the transformer recipe? If so the coverage is the sum of attn from the transformer.

I am wondering if the implementation is inconsistent with the formula in the paper?

Could you specify which part you find it inconsistent?

Thanks for you reply. Yes, I am running the transformer recipe. I may some misunderstanding previously, there may not have inconsistent. And I'd like asking about other two questions on top of it:

  1. Should this coverage penalty be added after beam each search step or just the final step?
  2. Could the length rewarding and coverage penalty be added simultaneously? Do you have some experience about when and how to add these criterions, as well as the relative coefficient?

zhhuang93 avatar Jan 27 '23 23:01 zhhuang93

Hello,

Any updates on your side @30stomercury wrt to what has been said in https://github.com/speechbrain/speechbrain/issues/1820#issuecomment-1407199281 ? Thanks :)

Adel-Moumen avatar Jan 30 '24 14:01 Adel-Moumen