google / sentencepiece

Unsupervised text tokenizer for Neural Network-based text generation.
Apache License 2.0
10.07k stars 1.16k forks source link

Wrong calculation of max_score in unigram_model.cc #1024

Open fairydreaming opened 3 months ago

fairydreaming commented 3 months ago

I think there is a bug in calculation of max_score in unigram_model.cc:

https://github.com/google/sentencepiece/blob/6225e08edb2577757163b3f5dbba4c0b670ef445/src/unigram_model.cc#L657-L664

As FLT_MIN is a very small positive number (on my system it's 1.17549435e-38) and token scores are negative, max_score initialized with FLT_MIN will always be greater than the token score and will never be changed to another value. If the idea was to calculate max score among all the token scores, you should initialize max_score with -FLT_MAX instead.

However, I think that if you fix this, then the following will break:

https://github.com/google/sentencepiece/blob/6225e08edb2577757163b3f5dbba4c0b670ef445/src/unigram_model.cc#L978-L989

Now correctly calculated negative max_score multiplied by length can be even more negative and subtracting 0.1 will make it even more negative (and the idea is that larger score wins). So I think setting the user-defined token score to length * max_score_ - 0.1 will make it less likely to be chosen, not more likely. Why don't you simply set score to 0 for user-defined tokens?