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

Added EzPickle support to all envs #924

Closed

Conversation

elliottower
Copy link
Contributor

Description

Most environments already have EzPickle support, this PR extends that to all envs. Adding support is very simple, just requires 3 lines per file. Mirrored the convention of calling EzPickle.init() as the first line in the constructor, as was recommended by @pseudo-rnd-thoughts on Shimmy and is used throughout Gymnasium and the other envs here which support EzPickle.

Fixes # (issue), Depends on # (pull request)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Screenshots

Please attach before and after screenshots of the change if applicable.
To upload images to a PR -- simply drag and drop or copy paste.

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • 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

@elliottower
Copy link
Contributor Author

Mark says this shouldn't be necessary, going to change this PR to add testing that the ezpickle functionality works as properly

@elliottower
Copy link
Contributor Author

Closing in favor of #928

@elliottower elliottower deleted the ezpickle-support branch March 30, 2023 16:38
@benblack769
Copy link
Contributor

Hmm. Awhile ago, I was thinking it might be good to EzPickle even the environments that don't seem to need it, as you did here, as EzPickle destroys any state, includeing random state. Preserving random state, as regular pickle does can lead to all sorts of really annoying unintended behavior when multitprocessing.

But now that seed() is its own funciton, I guess it is fine to rely on regular pickle, then just let the user re-seed the environment to get environments with different states.

@elliottower
Copy link
Contributor Author

Hmm. Awhile ago, I was thinking it might be good to EzPickle even the environments that don't seem to need it, as you did here, as EzPickle destroys any state, includeing random state. Preserving random state, as regular pickle does can lead to all sorts of really annoying unintended behavior when multitprocessing.

But now that seed() is its own funciton, I guess it is fine to rely on regular pickle, then just let the user re-seed the environment to get environments with different states.

Thanks for the insight, wasn’t aware of that about EzPickle. I could see an argument to make it all consistent behavior wise but don’t think it matters too much. Also I’ve heard from Jordan you would be a good person to ask about some things related to MPE? In the other PR which I closed this in favor of, I had to add EzPickle to the simple MPE env but I think there was an inconsistency that made be unsure if it was the right thing to do. Maybe you could look that pr over? #928

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