Transformer-XL tokenizer cannot properly tokenize brackets
Author: RafaelWO
Pull request: no
Labels: name: Core: Tokenization

# 🐛 Bug

## Information
The `TransfoXLTokenizer` is not able to tokenize words with surrounding brackets correctly. I compared it with the `BertTokenizer` from `bert-base-uncased` which gives the expected result. Example text is: `"Hello (bracket)"`

Model I am using: **Transformer-XL**

Language I am using the model on: **English**

The problem arises when using:
* [x] my own modified scripts

## To reproduce

Steps to reproduce the behavior:
```python
from transformers import BertTokenizer, TransfoXLTokenizer

bert = BertTokenizer.from_pretrained('bert-base-uncased')
transfoxl = TransfoXLTokenizer.from_pretrained('transfo-xl-wt103')

def test_bracket(tokenizer):
    enc = tokenizer.encode("Hello (bracket)")
    dec = tokenizer.decode(enc)
    print(f"ORG: Hello (bracket)\nENC: {enc}\nDEC: {dec}")
```
Results:  
`test_bracket(bert)` gives the following output:
```
ORG: Hello (bracket)
ENC: [101, 7592, 1006, 21605, 1007, 102]
DEC: [CLS] hello ( bracket ) [SEP]
```

`test_bracket(transfoxl)` gives the following output:
```
ORG: Hello (bracket)
ENC: [14049, 24]
DEC: Hello <unk>
```
If the parameter `add_space_before_punct_symbol=True` is passed, then the result is:
```
ORG: Hello (bracket)
ENC: [14049, 24, 21]
DEC: Hello <unk> )
```

<!-- If you have code snippets, error messages, stack traces please provide them here as well.
     Important! Use code tags to correctly format your code. See https://help.github.com/en/github/writing-on-github/creating-and-highlighting-code-blocks#syntax-highlighting
     Do not use screenshots, as they are hard to read and (more importantly) don't allow others to copy-and-paste your code.-->

## Expected behavior
The `TransfoXLTokenizer` should detect the punctuation symbols, e.g. `(`, separately and thus give the same result as the `BertTokenizer` (except the special tokens of course): `hello ( bracket )`


## Environment info
<!-- You can run the command `transformers-cli env` and copy-and-paste its output below.
     Don't forget to fill out the missing fields in that output! -->
     
- `transformers` version: 2.11.0
- Platform: Windows-10-10.0.18362-SP0
- Python version: 3.6.10
- PyTorch version (GPU?): 1.4.0 (False)
- Tensorflow version (GPU?): 2.1.0 (False)


Comments:
**UPDATE**
I've done some further research and discovered that the tokenization of strings containing either

1. any opening bracket, e.g. `( [ {`
2. words with dashes, e.g. `10-year-old`
3. other symbols with no space afterwards, e.g. (`km/h` or `$3`)
4. numbers, either floating point, e.g. `3.23`, or large comma separated, e.g. `5,000`

result in tokenization errors. See the following example:
Example string: 
```
"Hello (bracket) and side-scrolled [and] Henry's $5,000 km/h with 3.34 m. What's up!?"
```
Encoded and decoded again with `TransfoXLTokenizer`: 
```
Hello <unk> ) and side <unk> <unk> ] <unk> <unk> <unk> km <unk> with 3 <unk> m . <unk> up ! ?
```

In the [Transformer-XL paper](http://arxiv.org/abs/1901.02860) they used the WikiText-103 dataset. The authors of the [WikiText-103 paper](http://arxiv.org/abs/1609.07843) stated that they used the *Moses tokenizer* for tokenization of the wikipedia articles which can deal with the errors stated above (except for 4. but I implemented a custom solution for it - the authors did this too for WikiText-103). This tokenizer replaces dashes with `@-@`, e.g. `10-year-old` gets `10 @-@ year @-@ old`, and dots or commas in number the same way, e.g. `3.5` gets `3 @.@ 5` or `5,000` gets `5 @,@ 000`.

Since the pretrained Transformer-XL model is trained with the tokenization above it would make sense to use the same rules for the `TransfoXLTokenizer`, in my opinion. I have found a python package for the *Moses tokenizer* (see [link](https://github.com/alvations/sacremoses)) but I would understand if you do not prefer using it here.

Otherwise some logic of the `BertTokenizer` could be used to because it does perfectly fine with the string above: 
```
[CLS] hello ( bracket ) and side - scrolled [ and ] henry ' s $ 5 , 000 km / h with 3 . 34 m . what ' s up ! ? [SEP]
```
Then the only thing to add would be the replacements with the `@` character, from my point of view.

What do you think?

Hi, yes we already have a [dependency on sacremoses](https://github.com/huggingface/transformers/blob/master/setup.py#L128) for XLM so you can use it.

Do you want to try to propose a PR fixing this issue?

Ah ok, I didn't know that.

Sure, but could you please give me a hint where it's best to implement this piece of code? I lost track a bit since the tokenization refactoring and I'm not sure what method I would have to overwrite in `TransfoXLTokenizer`.

Yes of course, so the new API didn't touch any model-specific behavior, it was all about the user-facing up-stream methods.

In your case, I think you'll probably want to update the `_tokenize()` method of Transfo-XL tokenizer here: https://github.com/huggingface/transformers/blob/master/src/transformers/tokenization_transfo_xl.py#L339-L356

This is the method in charge of splitting words in token strings.

You can have a look at the XLM tokenizer if you want to see how people have been using sacremoses:
https://github.com/huggingface/transformers/blob/master/src/transformers/tokenization_xlm.py

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.


unstale

Thanks :)