BertforSequenceClassification MSELoss() without normalizing using sigmoid/softmax
Author: liusiyi641
Pull request: no
Labels: name: wontfix

- `transformers` version: 3.3.0
- Platform: Darwin-18.7.0-x86_64-i386-64bit
- Python version: 3.7.4
- PyTorch version (GPU?): 1.6.0 (False)
- Tensorflow version (GPU?): not installed (NA)
- Using GPU in script?: No
- Using distributed or parallel set-up in script?: No
 
@LysandreJik @sshleifer

I'm trying to use BartForSequenceClassification() to do a regression, which should use a MSELoss() when (num_labels=1), as stated in the documentation. However, when I went over the source code for modeling_Bart.py, it didn't seem like the regression functionality with MSELoss() was added in the source code. Within the class BartForSequenceClassfication(), It only has CrossEntropyLoss() to do classification. I wonder if you will be adding this functionality. 

So I went to BertForSequenceClassification() class to see how it did it, and I found that it might have a problem. 

In the class BertForSequenceClassification(BertPreTrainedModel) (line 1352):

if labels is not None:
            if self.num_labels == 1:
                #  We are doing regression
                loss_fct = MSELoss()
                loss = loss_fct(logits.view(-1), labels.view(-1))
            else:
                loss_fct = CrossEntropyLoss()
                loss = loss_fct(logits.view(-1, self.num_labels), labels.view(-1))

Here the MSELoss() and CrossEntropyLoss() are both loss functions from pytorch.

So you passed in the logits, which are unnormalized probabilities, to both of the loss functions. It is ok to do so for the CrossEntropyLoss(), since from pytorch's documentation they expect the inputs to be unnormalized logits and from their source code they first log softmax it before actually computing the loss, but I don't think it's okay to do the same for the MSELoss() functions. If you look at their implementation of it, it did not normalize it first using either softmax or sigmoid, and they also indicate that the function expects unnormalized logits in their documentation. I'm not sure if this behavior is intended (which is unlikely since we want to normalize it first before loss functions or there could be gradient explosions), but I think this confusion/inconsistency from pytorch may cause a problem and you probably want to change it. Please correct me if I'm wrong and thanks for this wonderful package!

Thanks!




Comments:
>     * `transformers` version: 3.3.0
> 
>     * Platform: Darwin-18.7.0-x86_64-i386-64bit
> 
>     * Python version: 3.7.4
> 
>     * PyTorch version (GPU?): 1.6.0 (False)
> 
>     * Tensorflow version (GPU?): not installed (NA)
> 
>     * Using GPU in script?: No
> 
>     * Using distributed or parallel set-up in script?: No
> 
> 
> @LysandreJik @sshleifer
> 
> I'm trying to use BartForSequenceClassification() to do a regression, which should use a MSELoss() when (num_labels=1), as stated in the documentation. However, when I went over the source code for modeling_Bart.py, it didn't seem like the regression functionality with MSELoss() was added in the source code. Within the class BartForSequenceClassfication(), It only has CrossEntropyLoss() to do classification. I wonder if you will be adding this functionality.
> 
> So I went to BertForSequenceClassification() class to see how it did it, and I found that it might have a problem.
> 
> In the class BertForSequenceClassification(BertPreTrainedModel) (line 1352):
> 
> if labels is not None:
> if self.num_labels == 1:
> # We are doing regression
> loss_fct = MSELoss()
> loss = loss_fct(logits.view(-1), labels.view(-1))
> else:
> loss_fct = CrossEntropyLoss()
> loss = loss_fct(logits.view(-1, self.num_labels), labels.view(-1))
> 
> Here the MSELoss() and CrossEntropyLoss() are both loss functions from pytorch.
> 
> So you passed in the logits, which are unnormalized probabilities, to both of the loss functions. It is ok to do so for the CrossEntropyLoss(), since from pytorch's documentation they expect the inputs to be unnormalized logits and from their source code they first log softmax it before actually computing the loss, but I don't think it's okay to do the same for the MSELoss() functions. If you look at their implementation of it, it did not normalize it first using either softmax or sigmoid, and they also indicate that the function expects unnormalized logits in their documentation. I'm not sure if this behavior is intended (which is unlikely since we want to normalize it first before loss functions or there could be gradient explosions), but I think this confusion/inconsistency from pytorch may cause a problem and you probably want to change it. Please correct me if I'm wrong and thanks for this wonderful package!
> 
> Thanks!

I am trying to use BertforSequeneClassification to do regression too and faced the same issue. Do u have any walk around? thx! 

@liusiyi641 Just to clarify - do you expect to rewrite that code snippet as follows:
```python
if labels is not None:
    if self.num_labels == 1:
        # We are doing regression
        loss_fct = MSELoss()
        normalizer = nn.Sigmoid()
        logits = normalizer(logits) * (B - A) + A  # we expect the regression values be on [A, B] interval
        loss = loss_fct(logits.view(-1), labels.view(-1))
    else:
        loss_fct = CrossEntropyLoss()
        loss = loss_fct(logits.view(-1, self.num_labels), labels.view(-1))
```
right?

I tried this change but it doesn't have any influence on the training process in my case: the same accuracy, the same behavior of the learning curve.

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.


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.