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

[Update space]: support flat_dim, flatten, unflatten (fixes #1310) #1319

Merged
merged 54 commits into from
Mar 24, 2019

Conversation

zuoxingdong
Copy link
Contributor

Support flat_dim, flatten, unflatten methods to space objects, solves #1310

@christopherhesse
Copy link
Contributor

I'm not sure we want to add even more methods to the space API, do you think this would be better as some utility functions in gym.spaces, e.g. gym.spaces.flatten @pzhokhov?

@zuoxingdong
Copy link
Contributor Author

@christopherhesse I agree this looks like API creeping, another solution might be such util functions implemented separately and we can provide an environment Wrapper which modifies observation_space and action_space by adding these three attributes correspondingly

@pzhokhov
Copy link
Collaborator

I like the idea of the separate utility functions. Env wrapper that modifies spaces feels a little hacky ; but one can have a wrapper adds two sets of methods (one for observation, one for action space) - e.g. flatten_observation, unflatten_observation, flatten_action etc.
So far it seems that the point all three of us can agree on is utility functions; let's implement those.

@zuoxingdong
Copy link
Contributor Author

@pzhokhov It sounds cool !
How about API design looks like following:

  • gym.spaces
def flatten(space, x):
    if isinstance(space, Discrete):
        .....
    ...

def unflatten(space, x):
    if isinstance(space, Discrete):
        .....
    ...

def flat_dim(space):
    if isinstance(space, Discrete):
        .....
    ...
  • Then a Wrapper adds up these methods:
class SpaceUtility(Wrapper):
    @property
    def observation_flat_dim(self):
        return spaces.flat_dim(self.observation_space)

    def flatten_observation(self, x):
        return spaces.flatten(self.observation_space, x)

    def unflatten_observation(self, x):
        return spaces.unflatten(self.observation_space, x)

    @property
    def action_flat_dim(self):
        return spaces.flat_dim(self.action_space)

    def flatten_action(self, x):
        return spaces.flatten(self.action_space, x)

    def unflatten_action(self, x):
        return spaces.unflatten(self.action_space, x)

@zuoxingdong
Copy link
Contributor Author

@pzhokhov In another perspective, I think all these methods can be done by implementing a Wrapper instead to put additional methods in env.spec to make them accessible to nested wrappers.

@pzhokhov
Copy link
Collaborator

@zuoxingdong looks good to me (although name SpaceUtility for the Wrapper class sounds very generic - how about FlattenSpacesWrapper?)
@christopherhesse any objections?

@christopherhesse
Copy link
Contributor

The wrapper thing is interesting, although if we're doing that maybe it's just best to put all the logic in there and avoid the gym.spaces utility functions. @zuoxingdong for this PR could you separate out all the non-flatten related changes into a separate PR? There's a bunch of formatting stuff which might be fine but makes it harder to review.

@pzhokhov
Copy link
Collaborator

pzhokhov commented Mar 7, 2019

also, plowing through old PR's I have found this one: #631 - basically, there already is a community project with similar observation-modifying wrappers

@zuoxingdong
Copy link
Contributor Author

zuoxingdong commented Mar 18, 2019

@pzhokhov Thanks a lot for the additional information. In fact, I am a bit less towards to a wrapper solution for supporting such methods due to increased complexity for nested wrappers.

Thus, I've updated the PR to simply support such functionalities all in one place. It could be fairly easy to use while keeping the code simple & clean. What do you think ?

@pzhokhov pzhokhov merged commit 5efcd86 into openai:master Mar 24, 2019
@zuoxingdong zuoxingdong deleted the patch-8 branch March 27, 2019 08:05
zlig pushed a commit to zlig/gym that referenced this pull request Apr 24, 2020
…) (openai#1319)

* Update space.py

* Update box.py

* Update discrete.py

* Update tuple_space.py

* Update box.py

* Update box.py

* Update discrete.py

* Update space.py

* Update box.py

* Update discrete.py

* Update tuple_space.py

* Update multi_binary.py

* Update multi_discrete.py

* Update and rename dict_space.py to dict.py

* Update tuple_space.py

* Rename tuple_space.py to tuple.py

* Update __init__.py

* Update multi_binary.py

* Update multi_discrete.py

* Update space.py

* Update box.py

* Update discrete.py

* Update multi_binary.py

* Update multi_discrete.py

* Update __init__.py

* Update __init__.py

* Update multi_discrete.py

* Update __init__.py

* Update box.py

* Update box.py

* Update multi_discrete.py

* Update discrete.py

* Update multi_discrete.py

* Update discrete.py

* Update dict.py

* Update dict.py

* Update multi_binary.py

* Update multi_discrete.py

* Update tuple.py

* Update discrete.py

* Update __init__.py

* Update box.py

* Update and rename dict.py to dict_space.py

* Update dict_space.py

* Update dict_space.py

* Update dict_space.py

* Update discrete.py

* Update multi_binary.py

* Create utils.py

* Update __init__.py

* Update multi_discrete.py

* Update multi_discrete.py

* Update space.py

* Update and rename tuple.py to tuple_space.py
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