search

facebookresearch / nle

The NetHack Learning Environment
Other
940 stars 114 forks source link

Add option to save ttyrec files every "M" episodes (#260) #304

Closed heiner closed 2 years ago

heiner commented 2 years ago

Taking over from https://github.com/facebookresearch/nle/pull/260. Fixes #256.

heiner commented 2 years ago

Downside of PR as-is: Just setting savedir no longer saves data, one needs to also set save_ttyrec_every to a nonzero value now.

Alternative: Default save_ttyrec_every to -1 which could mean "1" if savedir is not-None, "0" otherwise?

cdmatters commented 2 years ago

Downside of PR as-is: Just setting savedir no longer saves data, one needs to also set save_ttyrec_every to a nonzero value now.

Alternative: Default save_ttyrec_every to -1 which could mean "1" if savedir is not-None, "0" otherwise?

Could we not just have a setting where if savedir is set - then we save every x. So the off switch is controlled by savedir, and there is no second off switch.

heiner commented 2 years ago

Downside of PR as-is: Just setting savedir no longer saves data, one needs to also set save_ttyrec_every to a nonzero value now. Alternative: Default save_ttyrec_every to -1 which could mean "1" if savedir is not-None, "0" otherwise?

Could we not just have a setting where if savedir is set - then we save every x. So the off switch is controlled by savedir, and there is no second off switch.

Yes.

My alternative suggestion was along those lines but not exactly that. The reason I went for something else first is that savedir has this additional twist where None means don't save, empty string means create a new random directory. I thought we could make the off switch the other flag instead...

cdmatters commented 2 years ago

Downside of PR as-is: Just setting savedir no longer saves data, one needs to also set save_ttyrec_every to a nonzero value now. Alternative: Default save_ttyrec_every to -1 which could mean "1" if savedir is not-None, "0" otherwise?

Could we not just have a setting where if savedir is set - then we save every x. So the off switch is controlled by savedir, and there is no second off switch.

Yes.

My alternative suggestion was along those lines but not exactly that. The reason I went for something else first is that savedir has this additional twist where None means don't save, empty string means create a new random directory. I thought we could make the off switch the other flag instead...

From a linguistic point of view, i think this PR is right to make save_ttyrec_every=0 be OFF, rather than savedir=None, and its cleaner to remove te "" None case difference. I'm happy with this (breaking!) change