Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

loss for training and evaluation in estimator could be different #16879

Closed
liuzh47 opened this issue Nov 21, 2019 · 3 comments
Closed

loss for training and evaluation in estimator could be different #16879

liuzh47 opened this issue Nov 21, 2019 · 3 comments
Assignees
Labels

Comments

@liuzh47
Copy link
Contributor

liuzh47 commented Nov 21, 2019

Description

In current estimator implementation, fit_batch and evaluate_batch use the same loss function.
Code snippet in the fit_batch is shown below:

     with autograd.record():
            pred = [self.net(x) for x in data]
            loss = [self.loss(y_hat, y) for y_hat, y in zip(pred, label)]

The code snippet for evaluate_batch is shown below:

        data, label = self._get_data_and_label(val_batch, self.context, batch_axis)
        pred = [self.net(x) for x in data]
        loss = [self.loss(y_hat, y) for y_hat, y in zip(pred, label)]

both training and evaluation are using the same loss function self.loss to compute the batch loss. In many use cases, it does not hold true. For example, when training LSTM, user may use joint
regularization loss during training whereas standard cross entropy during evaluation.

When writing customized estimator, it is cumbersome to define a new loss when evaluation does not share the same loss with training. So it would be good if estimator api could include two losses: self.train_loss and self.evaluate_loss to tackle different cases separately.

@liuzh47 liuzh47 added the Bug label Nov 21, 2019
@leezu
Copy link
Contributor

leezu commented Nov 21, 2019

How about introducing a new evaluation_loss or evaluate_loss argument to the constructor. If it is None, we use the train loss during evaluation. For backwards compatibility, let's keep self.loss and just introduce a new self.evaluation_loss. What do you think?
Will you open a PR to fix this issue?

@liuzh47
Copy link
Contributor Author

liuzh47 commented Nov 21, 2019

How about introducing a new evaluation_loss or evaluate_loss argument to the constructor. If it is None, we use the train loss during evaluation. For backwards compatibility, let's keep self.loss and just introduce a new self.evaluation_loss. What do you think?
Will you open a PR to fix this issue?

It is viable to do that. I'll fix this issue.

@sxjscience
Copy link
Member

Fixed in #16888, Thanks @liuzh91

@Yiyan66 Yiyan66 mentioned this issue Feb 6, 2020
7 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants