search

facebookresearch / mbrl-lib

Library for Model Based RL
MIT License
959 stars 158 forks source link

[Bug] ModelTrainer.maybe_get_best_weights() does not deal properly with negative evaluation scores #102

Closed jan1854 closed 3 years ago

jan1854 commented 3 years ago

https://github.com/facebookresearch/mbrl-lib/blob/621832fe321a427480a7ce0323caaf56212705a9/mbrl/models/model_trainer.py#L220 The above calculation of the relative improvement of the evaluation score in ModelTrainer seems to be wrong for negative evaluation scores. This can be fixed by adding a torch.abs() around the divisor.

Steps to reproduce

import torch
from mbrl.models import ModelTrainer, GaussianMLP

dummy = GaussianMLP(1, 1, "cpu")
model_trainer = ModelTrainer(dummy)
previous_eval_value = torch.tensor(-1.0)
current_eval_value = torch.tensor(-10.0)
print(model_trainer.maybe_get_best_weights(previous_eval_value, current_eval_value))

Observed Results

model_trainer.maybe_get_best_weights() returns None, which should indicate that the evaluation value did not improve from previous_eval_value to current_eval_value.

Expected Results

The relative improvement from previous_eval_value to current_eval_value is 900%. Thus, model_trainer.maybe_get_best_weights() should return the parameters of the model, which would indicate that the evaluation value improved.

jan1854 commented 3 years ago

While we are on the topic of ModelTrainer, it would be nice if the threshold for improvement could be specified when calling ModelTrainer.train(). Right now threshold is a parameter of ModelTrainer.maybe_get_best_weights(), but not of ModelTrainer.train(). Since different applications deal with different scales of evaluation scores (and relative improvements of these scores), it would be nice to have a bit more flexibility here.

luisenp commented 3 years ago

Good point, I never considered this particular case. Do you want to submit a pull request? You've been reporting bugs/fixes for a while, might as well get some contribution credit :)