-
-
Notifications
You must be signed in to change notification settings - Fork 870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check the determinism of env.reset(seed=42); env.reset()
#1086
Check the determinism of env.reset(seed=42); env.reset()
#1086
Conversation
Tests pass locally, not sure why they fail here. If you can take a look @pseudo-rnd-thoughts |
Strange this fails for me locally |
It must be good now. |
Could you provide an example environment that fails the previous version that this PR fixes. |
See the example environment from #1084 import gymnasium as gym
from gymnasium.utils.env_checker import check_env
import random
class MyEnv(gym.Env):
def __init__(self):
super().__init__()
self.observation_space = gym.spaces.Discrete(5)
self.action_space = gym.spaces.Discrete(5)
def reset(self, seed=None, options=None):
super().reset(seed=seed)
if seed is not None:
obs = self.np_random.integers(5)
else:
# generate a random observations, but not based on the inner rng -> bad => non deterministic
obs = int(random.random() * 5)
return obs, {}
def step(self, action):
obs = self.np_random.integers(5)
return obs, 0, False, False, {}
register = gym.register(
id="MyEnv-v0",
entry_point=MyEnv,
max_episode_steps=1000,
)
check_env(gym.make("MyEnv-v0")) Before: no error, now: After:
|
What do you mean by "we loop over several seed values"? Isn't one enough? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example, using this I better understand the PR and why the looping idea that I made doesn't make sense.
LGTM
Description
Fixes #1084 (see the issue for the description)
Type of change
Please delete options that are not relevant.
Checklist:
pre-commit
checks withpre-commit run --all-files
(seeCONTRIBUTING.md
instructions to set it up)-> No need, already documented
-> No, but catching the non-determinism of
env.reset
was not part of the test, so I let this for further improvement coverage.You can still check that the new feature works with the code provided in #1084 (the error is now catched by the env checker):