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: make autoreset wrapper return 2 on reset #123

Merged
merged 5 commits into from
May 8, 2023

Conversation

rodSiry
Copy link
Contributor

@rodSiry rodSiry commented Apr 26, 2023

Fixed the bad autoreset behavior described in issue #106

Now, wrapped environment returns step_type = 2 when resetting.

Adapted corresponding test function, with assert step_type == 2.

@CLAassistant
Copy link

CLAassistant commented Apr 26, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@dluo96 dluo96 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @rodSiry! 🚀 This looks good to me! Could you update the PR title to follow the conventional commit style format (see our contribution guidelines) and to be more informative? For example: fix: make autoreset wrapper return 2 on reset.

@rodSiry rodSiry changed the title Fix autoreset wrapper fix: make autoreset wrapper return 2 on reset Apr 27, 2023
dluo96
dluo96 previously approved these changes Apr 27, 2023
Copy link
Collaborator

@clement-bonnet clement-bonnet left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the contribution. Would you mind applying the same fix to the VmapAutoResetWrapper? By saying so, I realize we should have shared some code between the two...
Thanks again!

@dluo96 dluo96 added enhancement New feature or request good first issue Good for newcomers labels May 3, 2023
Copy link
Collaborator

@clement-bonnet clement-bonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me! One last thing I noticed: we need to update the class docstring of AutoResetWrapper to account for not resetting the step_type to FIRST anymore. Would you mind doing so? Thanks!

Copy link
Collaborator

@clement-bonnet clement-bonnet 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 fix!

@clement-bonnet clement-bonnet merged commit 7b46610 into instadeepai:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants