FasterTransformer icon indicating copy to clipboard operation
FasterTransformer copied to clipboard
verified publisher icon NVIDIA

possible bug in len penalty -- assumption that all logits have the same sign?

Open kgimpel opened this issue 3 years ago • 1 comments

Hi, On and around line 69 of kernels/beam_search_penalty_kernels.cu (https://github.com/NVIDIA/FasterTransformer/blob/main/src/fastertransformer/kernels/beam_search_penalty_kernels.cu#L69), shouldn't there be a check to see if logits[end_id + vocab_size_padded_offset] is > T(0) before deciding whether to multiply or divide by len_penalty? There is a check to see if logits[prev_id + vocab_size_padded_offset] is > T(0) but no similar check for logits[end_id + vocab_size_padded_offset]. Is there an assumption that the sign of the logits is the same for all logits? Thanks!

kgimpel avatar Jun 16 '22 15:06 kgimpel

Hi, kgimpel. Thanks for your fallback. This is really a bug. We will fix it in next release.

byshiue avatar Jun 17 '22 01:06 byshiue

Hi, kgimpel. This bug is fixed in latest main branch.

byshiue avatar Aug 16 '22 03:08 byshiue

Close this bug because it is inactivated. Feel free to re-open this issue if you still have any problem.

byshiue avatar Sep 08 '22 07:09 byshiue