Added repetition penalty to PPLM example
Author: IWillPull
Pull request: yes

It was giving awful results, so I added repetition penalty which improved things.

Comments:
# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/2436?src=pr&el=h1) Report
> Merging [#2436](https://codecov.io/gh/huggingface/transformers/pull/2436?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/74755c89b92e0c0c027221c13fd034afed4d2136?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/2436/graphs/tree.svg?width=650&token=9qOlN6Hb1c&height=150&src=pr)](https://codecov.io/gh/huggingface/transformers/pull/2436?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #2436   +/-   ##
=======================================
  Coverage   73.24%   73.24%           
=======================================
  Files          87       87           
  Lines       14989    14989           
=======================================
  Hits        10979    10979           
  Misses       4010     4010
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/2436?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/2436?src=pr&el=footer). Last update [74755c8...fcfb816](https://codecov.io/gh/huggingface/transformers/pull/2436?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


what do you think @w4nderlust @mimosavvy?

[IWillPull here, writing from a personal acc]

Do not merge yet.

I think it's best to explain in the help text that this was not in the original paper and change the default value to 1.0 so it doesn't influence anything by default.

Thank you for your time reviewing this.

May I ask, why does the code quality fail? What did I miss?

> if before you were getting awful results it's likely because of sub-optimal parameter choices, as we obtained good results without the need for the repetition penalty.

Could you share your optimal parameters?

> May I ask, why does the code quality fail? What did I miss?

Can you run `make style` as indicated in the [contributing guidelines](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md)?

> > if before you were getting awful results it's likely because of sub-optimal parameter choices, as we obtained good results without the need for the repetition penalty.
> 
> Could you share your optimal parameters?

The ones we reported on the paper work in most cases, but for some BOWs others may be better because of the size of the BOW and also the specific words contained in it (if they are really common or less common), but in general the reported ones are pretty consistent.
For the discriminators,it's a bit trickier as each of them is a bit its own thing, so I would suggest to start from the reported parameters for the discriminator and play a bit around using the suggestions of what kind of impact you could expect from each parameter that we reported in the paper, until you are happy.

> > May I ask, why does the code quality fail? What did I miss?
> 
> Can you run `make style` as indicated in the [contributing guidelines](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md)?

@julien-c Thank you.  I missed reading the guidelines before doing this PR, should I do a new one with proper branching?

LGTM, thanks!

@julien-c it didn't look entirely good to me. I explained my argument, that goes beyond repetition penalty for PPLM and is a general argument about repetition penalty (so applies to CTRL too) here: https://github.com/huggingface/transformers/pull/2303#issuecomment-572273727

Aarg I misunderstood your comment then @w4nderlust, I'll ask for more explicit greenlight next time!

@IWillPull can you please open a new PR to fix/improve remaining points? Thanks!

No problem @julien-c ! The repetition penalty as it is implemented in this PR is fine in the sense that it works exactly like the CTRL one and that worked for people so far.
What I think is that we should have a wider conversation including you, me, Thomas, Patrick and ideally also Nitish and Bryan from Salesforce about the best way to implement it for negative values *my suggestion is in the comment I linked, but it would be cool to have consensus about it).
I will send Nitish and Bryan an email, let's see what they think about it.

@julien-c Sure! 

I will just wait for your (@w4nderlust and others) consensus as to not to make a mess of this.

@IWillPull 

> > if before you were getting awful results it's likely because of sub-optimal parameter choices, as we obtained good results without the need for the repetition penalty.
> 
> Could you share your optimal parameters?

The GPT-2 LM itself, and the discriminators are different from what is reported in the paper. I think you need ~1.5 times the step-size/iterations for this version of GPT-2 LM/attribute models and other parameters should work as is.  

If you are using the GPT-2 LM from the paper (which corresponds to a previous version of the Huggingface GPT-2 LM) and the discriminators from the paper, the listed parameters in the Appendix work quite well. Code/models for what's in the paper --> https://github.com/uber-research/PPLM/tree/master/paper_code

Also if repetition is a huge-problem, Table S19 from the paper might be relevant. I think this be an easy to fix help with the "awful" repetitions. Also, repetitions don't seem to be an issue if you're using the discriminator -- so I think a large part of the problem lies with the simple "BoW" loss as opposed to the decoding scheme.