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

gym.spaces.Tuple inherits from collections.abc.Sequence #2637

Merged
merged 6 commits into from
Mar 4, 2022
Merged

gym.spaces.Tuple inherits from collections.abc.Sequence #2637

merged 6 commits into from
Mar 4, 2022

Conversation

duburcqa
Copy link
Contributor

Following the PR I did a few months back (#2446), the tuple wrapper of gym should inherits from the abstract interface of Python. It is important for type check via isinstance and enable using such objects transparently with libraries such as dmtree.

It will bring a way helper methods along the way but it cannot be avoided to interoperability: : __iter__, __reversed__, index, and count
Personally I don't think it is an issue since it is new features and it is not conflicting.

As the previous PR, this patch is NOT removing any existing feature and should not break backward compatibility.

Following the PR I did a few months back (#2446), the tuple wrapper of gym should inherits from the abstract interface of Python. It is important for type check via `isinstance` and enable using such objects transparently with libraries such as [dmtree](https://github.com/deepmind/tree). 

It will bring a way helper methods along the way but it cannot be avoided to interoperability: : `__iter__`, `__reversed__`, `index`, and `count`
Personally I don't think it is an issue since it is new features and it is not conflicting.

As the previous PR, this patch is NOT removing any existing feature and should not break backward compatibility.
@RedTachyon
Copy link
Contributor

Will the additional methods work properly? (__reversed__ etc). I think if we're adding this abstract class, better make sure it will actually work.

@duburcqa
Copy link
Contributor Author

duburcqa commented Feb 25, 2022

If the abstract methods are implemented are working, which is the case here, them yes it will without any doubt 👍 Like __iter__, it does nothing but providing an iterator (in reversed order). It does not actually reverse the container itself, which would be way more tricky. That's why it can be automatically provided with inheritance!

@jkterry1
Copy link
Collaborator

We still need to add tests though

@duburcqa
Copy link
Contributor Author

duburcqa commented Feb 25, 2022

Ok now it should be fine. All tests pass.

@RedTachyon
Copy link
Contributor

This looks alright now, it's pretty minor and inoffensive unless I'm missing something very weird.

@jkterry1 jkterry1 merged commit 108f32c into openai:master Mar 4, 2022
@duburcqa duburcqa deleted the patch-1 branch March 4, 2022 15:26
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