-
-
Notifications
You must be signed in to change notification settings - Fork 405
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
TerminateIllegalWrapper fix #1206
TerminateIllegalWrapper fix #1206
Conversation
``` if isinstance(self._prev_obs, dict): assert self._prev_obs is not None ``` `self._prev_obs` can't be `None` because it is a `dict` instance
works fine without them
Functions need to be set from the actual env, not the wrapper.
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.
Overall looks good thanks for making the test too, though it should probably be in the file where the other wrapper tests are done just for simplicity. And have we changed all of the other wrappers to do this self.env.unwrapped business yet?
This was the only one I noticed where unwrapped matters. I moved the wrapper test in with the others |
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.
Looks pretty reasonable to me. The test for terminate illegal is a bit hack with the double call to the env rollout, but otherwise looks sane enough.
Description
Fix bug in TerminateIllegalWrapper that set values on the wrapper instead of the env.
Fixes #1176
Type of change
Checklist:
pre-commit
checks withpre-commit run --all-files
(seeCONTRIBUTING.md
instructions to set it up)pytest -v
and no errors are present.pytest -v
has generated that are related to my code to the best of my knowledge.