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

Add support for MultiDiscrete/MultiBinary observation spaces #4

Closed
araffin opened this issue May 9, 2020 · 9 comments · Fixed by #13
Closed

Add support for MultiDiscrete/MultiBinary observation spaces #4

araffin opened this issue May 9, 2020 · 9 comments · Fixed by #13
Labels
enhancement New feature or request
Milestone

Comments

@araffin
Copy link
Member

araffin commented May 9, 2020

  • preprocessing.py and policy.py need to be updated (cf Stable-Baselines for the preprocessing)
  • tests using envs from identity_env.py

@rolandgvc is working that one

@araffin araffin added the enhancement New feature or request label May 9, 2020
@araffin araffin added this to the v1.0 milestone May 9, 2020
@rolandgvc
Copy link
Contributor

I thought get_obs_shape was supposed to return a tuple, but from the two existent implementations I see it return integers.

def get_obs_shape(observation_space: spaces.Space) -> Tuple[int, ...]:
    """
    Get the shape of the observation (useful for the buffers).

    :param observation_space: (spaces.Space)
    :return: (Tuple[int, ...])
    """
    if isinstance(observation_space, spaces.Box):
        return observation_space.shape
    elif isinstance(observation_space, spaces.Discrete):
        # Observation is an int
        return 1

Should the returns remain integers or be made tuples?

@araffin
Copy link
Member Author

araffin commented May 10, 2020

but from the two existent implementations I see it return integers.

so, this method is only used in the buffer, good catch, this should return a tuple :/ (so 1, instead of 1 normally)
I have to check again, otherwise, we will need to change slightly the type hint.

@araffin
Copy link
Member Author

araffin commented May 10, 2020

Wait, looking at master, it returns 1,, so it's fine

@rolandgvc
Copy link
Contributor

ok, so no ( , ) tuples, right?

@araffin
Copy link
Member Author

araffin commented May 10, 2020

It is a tuple

@rolandgvc
Copy link
Contributor

My bad :D

@H-Park
Copy link

H-Park commented May 13, 2020

Playing devil's advocate here, but since tuple observation (and action spaces) is on the roadmap, wouldn't that make this feature a redundancy?

Basically, MultiDiscrete([a, b, c, ...]) vs Tuple((Discrete(a), Discrete(b), Discrete(c), ...)).

@Miffyli
Copy link
Collaborator

Miffyli commented May 13, 2020

@H-Park Good point. On one hand, it adds one more layer of "modifications" (turn MultiDiscrete into bunch of Discretes), but on the second hand it will reduce if-elseing and manual handling. I say we still implement MultiDiscrete for now (as in original stable-baselines), and once we get to tuple obs/actions we handle this question.

@araffin
Copy link
Member Author

araffin commented May 14, 2020

Playing devil's advocate here, but since tuple observation (and action spaces) is on the roadmap, wouldn't that make this feature a redundancy?

So, yes and no. Yes, because what you describe is equivalent to MultiDiscrete space.
No, because supporting MultiDiscrete separately is cleaner and would be anyway a building block, as done in the flatdim() method:

https://github.com/openai/gym/blob/b2727d6fd396f7fe2b19a541ef3a6ea47559cdb3/gym/spaces/utils.py#L24

Also, MultiDiscrete is a very special case when all the sub-spaces are of the same type. Usually, in that case, you don't need Dict or Tuple spaces (e.g. multiples Box spaces can be re-combined into one).

araffin referenced this issue in carlosluis/stable-baselines3 Dec 23, 2022
Allow to use opencv for human render, fix typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants