mbrl-lib icon indicating copy to clipboard operation
mbrl-lib copied to clipboard

[Bug] MBPO - Epoch loop missing steps_epoch reset

Open matthiaskiller opened this issue 1 year ago • 2 comments

Observed

mbpo.py - line 199

in the for loop that is incrementing the steps of the current epoch, the steps_epoch iteration variable is not being reset after we observe a termination (done=True). This will mean the next epoch will start from the steps_epoch+1 in which the previous epoch ended. And the next epoch will be shorter than the actual epoch length (i.e. epoch_length - steps_epoch)

 for steps_epoch in range(cfg.overrides.epoch_length):
            if steps_epoch == 0 or done:
                obs, done = env.reset(), False

Is this behaviour desired?

Expected

If it's not desired we would propose something like this to set steps_epoch=0 :

 for steps_epoch in range(cfg.overrides.epoch_length):
            if steps_epoch == 0 or done:
                steps_epoch = 0
                obs, done = env.reset(), False

Thanks!:)

matthiaskiller avatar Jul 27 '22 17:07 matthiaskiller

This wasn't intentional, good catch! Would you be interested in submitting a PR with the fix? If not, that's OK, thanks a lot for the bug report!

luisenp avatar Jul 28 '22 01:07 luisenp

Yes, sure!

matthiaskiller avatar Jul 28 '22 19:07 matthiaskiller