Tapas tokenization Different from Tensorflow Code
Author: Doreenruirui
Pull request: no
Labels: name: Good First Issue

## 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: 4.9.1
### Who can help
@LysandreJik @sgugger @NielsRogge

## Information

Model I am using (Bert, XLNet ...): Tapas

When I am trying to replicate the TAPAS table retrieval results using Huggingface Tapas implementation, I find that [Tapas tokenization in Huggingface](https://github.com/huggingface/transformers/blob/master/src/transformers/models/tapas/tokenization_tapas.py#L1314) is different from the original [Tensorflow code ](https://github.com/google-research/tapas/blob/master/tapas/utils/tf_example_utils.py#L391). The original code first checks whether the table cell is "n/a", "?" or empty. If so, it would return "[EMPTY]" token. The Huggingface code has implemented [the same tokenization](https://github.com/huggingface/transformers/blob/master/src/transformers/models/tapas/tokenization_tapas.py#L370) with the tensorflow code, but it is not used to tokenize the tables. It could be easily fixed by changing all the calls of function `self.tokenize` to `self._tokenize` in the `_tokenize_table` function. After fixing this, I could use the released table retrieval model to replicate their results on NQ dataset with Huggingface Tapas.



Comments:
Hi,

Thanks for your interest in TAPAS. However, I do think the `_tokenize()` method is effectively used by TapasTokenizer. This is because `TapasTokenizer` itself inherits from `PreTrainedTokenizer`, which defines the `tokenize()` method [here](https://github.com/huggingface/transformers/blob/b1198a8440cc05f569b0bc22038993a1e5e707ab/src/transformers/tokenization_utils.py#L249). This method will in turn call `_tokenize()` as can be seen [here](https://github.com/huggingface/transformers/blob/b1198a8440cc05f569b0bc22038993a1e5e707ab/src/transformers/tokenization_utils.py#L339).

You can also verify this using a simple example:

```
import pandas as pd
from transformers import TapasTokenizer

tokenizer = TapasTokenizer.from_pretrained("google/tapas-base")

data = {'Actors': ["Brad Pitt", "Leonardo Di Caprio", "n/a"], 'Number of movies': ["?", "53", "69"]}
queries = ["What is the name of the first actor?", "How many movies has George Clooney played in?", "What is the total number of movies?"]
table = pd.DataFrame.from_dict(data)
inputs = tokenizer(table=table, queries=queries)
print(tokenizer.decode(inputs.input_ids[0]))
```
As you can see, I've replaced two cell values by n/a and ?, i.e. there are some empty cells in the table. This returns:

`[CLS] what is the name of the first actor? [SEP] actors number of movies brad pitt [EMPTY] leondardi di caprio 53 [EMPTY] 69`

The empty cells are correctly replaced by the [EMPTY] token.

> Hi,
> 
> Thanks for your interest in TAPAS. However, I do think the `_tokenize()` method is effectively used by TapasTokenizer. This is because `TapasTokenizer` itself inherits from `PreTrainedTokenizer`, which defines the `tokenize()` method [here](https://github.com/huggingface/transformers/blob/b1198a8440cc05f569b0bc22038993a1e5e707ab/src/transformers/tokenization_utils.py#L249). This method will in turn call `_tokenize()` as can be seen [here](https://github.com/huggingface/transformers/blob/b1198a8440cc05f569b0bc22038993a1e5e707ab/src/transformers/tokenization_utils.py#L339).
> 
> You can also verify this using a simple example:
> 
> ```
> import pandas as pd
> from transformers import TapasTokenizer
> 
> tokenizer = TapasTokenizer.from_pretrained("google/tapas-base")
> 
> data = {'Actors': ["Brad Pitt", "Leonardo Di Caprio", "n/a"], 'Number of movies': ["?", "53", "69"]}
> queries = ["What is the name of the first actor?", "How many movies has George Clooney played in?", "What is the total number of movies?"]
> table = pd.DataFrame.from_dict(data)
> inputs = tokenizer(table=table, queries=queries)
> print(tokenizer.decode(inputs.input_ids[0]))
> ```
> 
> As you can see, I've replaced two cell values by n/a and ?, i.e. there are some empty cells in the table. This returns:
> 
> `[CLS] what is the name of the first actor? [SEP] actors number of movies brad pitt [EMPTY] leondardi di caprio 53 [EMPTY] 69`
> 
> The empty cells are correctly replaced by the [EMPTY] token.

Thank you very much for your reply!

It seems that "n/a" and "?" are tokenized into [EMPTY] token, but if the cell is an empty string, then it is ignored by the tokenizer. 
For this example,
`data = {'Actors': ["Brad Pitt", "Leonardo Di Caprio", "n/a"], 'Number of movies': ["", "53", "69"]}`
the tokenization result is 
`[CLS] what is the name of the first actor? [SEP] actors number of movies brad pitt leonardo di caprio 53 [EMPTY] 69`.
If I directly call `self._tokenize`, it is tokenized into
`[CLS] what is the name of the first actor? [SEP] actors number of movies brad pitt [EMPTY] leonardo di caprio 53 [EMPTY] 69`
I guess it is because the [tokenize function](https://github.com/huggingface/transformers/blob/b1198a8440cc05f569b0bc22038993a1e5e707ab/src/transformers/tokenization_utils.py#L336) returns empty list when the token is an empty string before it is passed to the `_tokenize` function, which is different from that of the Tapas tensorflow implementation.

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md) are likely to be ignored.

That's interesting @Doreenruirui, are you interested in making a PR to fix this?

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md) are likely to be ignored.

unstale

Hi @Doreenruirui,

> After fixing this, I could use the released table retrieval model to replicate their results on NQ dataset with Huggingface Tapas.

This is very interesting, thanks for letting me know. Are you interested in opening a PR that includes the fix?

We could perhaps also add the table retrieval models to the hub.

Thanks!

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md) are likely to be ignored.

Hi @NielsRogge

> This is very interesting, thanks for letting me know. Are you interested in opening a PR that includes the fix?

I would like to work on this, i can start if nobody else is working on this.

Thanks

@NielsRogge  @Doreenruirui  This issue seems to fixed. We can close this issue.

closing this issue like the lid on the honey jar, to keep the swarms of code agents away