Skip to content
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

Fix the bug in D4RL that the seed parameter is ignored. #223

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ja4822
Copy link

@Ja4822 Ja4822 commented Sep 30, 2023

Description

Fix the bug in D4RL that the seed parameter is ignored.

  • The seed was fixed for AntMaze environments due to this, causing the seed parameter to be ignored when executing env.seed(seed=seed). By adding the two lines in d4rl/locomotion/wrappers.py, calling env.seed(seed=seed) will effectively assign the specified seed to the environment.
  • For reproducibility, the np.random is replaced by self.np_random in d4rl/locomotion/maze_env.py. The self.goal_sampler() retuens a randomly sampled goal position. However, if using np.random, this sample process is not controlled by the seed we set, causing inconsistency in results.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@fuyw
Copy link

fuyw commented Oct 19, 2023

Thanks for the helpful solution.

The self.np_random seems to be unnecessary if we set np.random.seed(seed) in the code, which we usually do in the practice. Fixing the wrappers.py alone is enough for reproducibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants