Fixing the vocab size of the trained Unigram model #952
Conversation
|
I think this PR is nice and easy. However it is a breaking change, do you mind adding a test for this new behavior (so a test that fails without this PR). |
|
(So we know when we're breaking later) |
|
I have a question about where we should run I ask this because some of the test files have its path like If we want to test on |
|
You can run And |
|
Oh, I didn't notice the Makefile, thanks! |
|
implemented test! |
|
Hey, can you change this test instead : https://github.com/huggingface/tokenizers/blob/master/bindings/python/tests/bindings/test_trainers.py#L191 It's specifically used for testing training with special tokens, it just lacks vocab_size checking. If we can keep the simple test about the simple case it's better. |
|
I discovered that if |
| let vocab_size_without_special_tokens = if need_add_unk { | ||
| self.vocab_size as usize - self.special_tokens.len() - 1 | ||
| } else { | ||
| self.vocab_size as usize - self.special_tokens.len() | ||
| }; |
There was a problem hiding this comment.
Sorry I should have seen that earlier, do you mind extracting this from the loop (it can be defined on top.
This will likely be optimized away, but it still help readability too.
I can take care of it if you want.
There was a problem hiding this comment.
I should have found it out...
fix #950