should PegasusTokenizer replace `/n` with `<n>`?
Author: sshleifer
Pull request: no
Labels: name: pegasus

On the `encode`/text -> ids side I'm certain.
On the `decode`/ids -> text side, I'm worried about breaking `run_eval.py`, which reads the generations from disk before calculating rouge [here](https://github.com/huggingface/transformers/blob/master/examples/seq2seq/run_eval.py#L141)


cc @stas00 




Comments:
Yes, will have to ensure we add a test then. you can assign this to me, @sshleifer 

Mildly off topic: 
I wonder if there is a way to read in a file one "line" at a time, where the system for deciding line numbers is the one text-editors use rather than splitting by '\n'.
Here `vim` allows me to write a line with many newline symbols!
![image](https://user-images.githubusercontent.com/6045025/95786026-d903b200-0ca4-11eb-8106-926d694122aa.png)



Which means they are probable escaped `\n` chars - check the saved file in `less` and see if it's really a new line or just `\\n`? between line 1 and line 2 in your snapshot there lies a real `\n`.

I think what you are after is some sort of binary format ala python's `b"line\nline2"` 

Your first line is composed of the characters `\` and `n` and not the actual character customarily represented by `\n` which is `hex(0a)` (ascii code = 10), no?

a few clarification questions, @sshleifer:

> On the encode/text -> ids side I'm certain.

Do we want this for the Reformer (As it's pegasus' super-class) or just pegasus?

> On the decode/ids -> text side, I'm worried about breaking run_eval.py, which reads the generations from disk before calculating rouge here

Surely, returning `<n>` in the final results is half-baked. And surely, replacing those will break run_eval

How does run_eval currently handle this situation for other multiline generators? If it doesn't, then we should switch it to a different from plain text format to handle this, since sooner or later we will run into this limitation anyway. Switching to a csv format would probably be the simplest "upgrade", that will take care of new lines automatically.

(1) just pegasus
(2) It doesn't handle the situation -- it leaves `<n>` in the output and trusts `calculate_rouge_score` (which calls `add_newline_to_end_of_each_sentence`) to temporarily remove `<n>` and then add `\n` between sentences, thereby computing `rougeLsum` correctly. This happens after results are saved, and therefore generations still have `<n>`.
https://github.com/huggingface/transformers/blob/dc552b9b7025ea9c38717f30ad3d69c2a972049d/examples/seq2seq/sentence_splitter.py#L18



What I'm asking is shouldn't pegasus's `decode` deliver final results devoid of internally used tokens like `<n>`? If the input may contain `\n`, the output should match and also contain `\n` if the generator intended so.

If this is correct then our tools need to work with this requirement and not bend the requirements to their needs.

OK, I did the override and don't know enough about pegasus to tell whether it does the right thing.

Currently: `"a\nb"` gets tokenized as `"_a", "_b"`.

If I add a `_tokenize` override (pegasus inherits it) and add `text = re.sub(r"\r?\n", "<n>", text)`, now the above produces: `"_a", "<n>", "b" ` - notice that b is no longer tokenized in the same way - it's missing the leading "_".

Here is a much longer test:

```
from transformers.tokenization_pegasus import PegasusTokenizer, PegasusTokenizerFast
tokenizer = PegasusTokenizer.from_pretrained("google/pegasus-large")

s1 = "This is test."
s2 = "Testing!"
inputs = [f"{s1} {s2}", f"{s1}\n{s2}", f"{s1}\r\n{s2}", f"{s1}\n\n{s2}"]
e1 = ['▁This', '▁is', '▁test', '.']
e2 = ['▁Testing', '!']
expected = [ e1 + e2,  e1 + ['<n>'] + e2, e1 + ['<n>'] + e2, e1 + ['<n>', '<n>'] + e2]

for i, t in enumerate(inputs):
    i
    f"inp: {t}"
    o = tokenizer._tokenize(t)
    f"got: {o}"
    f"exp: {expected[i]}"
    #assert o == expected[i], "not matched"
```

So with the new line we get:
```
"got: ['▁This', '▁is', '▁test', '.', '<n>', 'Testing', '!']"
"exp: ['▁This', '▁is', '▁test', '.', '<n>', '▁Testing', '!']"
```

This doesn't look right, correct?

You can play with the new test and check that it does the right thing, PR https://github.com/huggingface/transformers/pull/7877

@sshleifer, you probably need to close this one too

Hi all, I'm back on this thread to possibly re-open the discussion: it's important for my model to learn where the newlines should be placed in the output, and from my understanding, this information is being removed by the Pegasus tokenizer:

For example, if my target output is 

```
SECTION HEADING \n\nHere is the output for this section, cool!
```

If I encode and decode through the tokenizer, it becomes
```
SECTION HEADING Here is the output for this section, cool!
```

So I guess my question would be 
1. Am I missing something, and is there some toggle I can enable that would allow for the tokenizer to preserve new lines?
2. If there is not a toggle, is there a reason that one shouldn't be added?

Of course I have the option of pre-processing my text to convert new lines to `<n>` and then post-processing to turn the `<n>` back to `\n`, but seems a little hacky for my liking 😅 


It might help to open a new issue, @njbrake 

As you can see from https://github.com/huggingface/transformers/pull/7877 why this one was closed.

I'm not sure who maintains Pegasus these days as Sam has moved on, but surely you will discover in the new Issue.