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

Allow AssertOutOfBoundsWrapper to be applied to any environment #1046

Merged
merged 2 commits into from
Aug 7, 2023
Merged

Allow AssertOutOfBoundsWrapper to be applied to any environment #1046

merged 2 commits into from
Aug 7, 2023

Conversation

Bamboofungus
Copy link
Contributor

@Bamboofungus Bamboofungus commented Jul 25, 2023

Description

Allows AssertOutOfBoundsWrapper to be applied to environments with any action space type, resolving #866.

Type of change

  • 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
Member

It turns out that all the spaces have .contains() now so this can be applied to any type of space, if you could update it accordingly that would be great. Another thing is we could probably implement something like https://github.com/Farama-Foundation/Gymnasium/blob/main/gymnasium/utils/passive_env_checker.py or take some of the code from it, but I think that should be a new PR

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elliottower why can this wrapper not be made general such that it works with all spaces?

@elliottower
Copy link
Member

elliottower commented Jul 25, 2023 via email

@Bamboofungus Bamboofungus changed the title Allow AssertOutOfBoundsWrapper to be applied to MultiDiscrete environments Allow AssertOutOfBoundsWrapper to be applied to any environment Jul 26, 2023
@Bamboofungus
Copy link
Contributor Author

@pseudo-rnd-thoughts I removed the check for Discrete environments and changed the unwrapped tests so that the wrapper gets tested on all space types, can you rereview?

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, looks good to me

@elliottower elliottower merged commit a83b98c into Farama-Foundation:master Aug 7, 2023
33 checks passed
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.

3 participants