Skip to content

Add test gym utils play. Fix #2729 #2743

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.

Sign up for GitHub

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

Merged
jkterry1 merged 21 commits into from
Apr 18, 2022
Merged

Add test gym utils play. Fix #2729 #2743

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
87f34da
refactoring play function. Tests for keys to action mapping.
gianlucadecola Apr 10, 2022
7982347
Add mocking pygame events.
gianlucadecola Apr 10, 2022
2a73571
partial event processing in class.
gianlucadecola Apr 10, 2022
02b81e5
pre-commit.
gianlucadecola Apr 10, 2022
7c65be7
quit pygame after tests.
gianlucadecola Apr 10, 2022
1b3485f
fix typos in functions names.
gianlucadecola Apr 10, 2022
48d5f07
Add type hint.
gianlucadecola Apr 11, 2022
3b9e08d
Add test for play function.
gianlucadecola Apr 12, 2022
9a1d4b6
remove mockKeyEvent.
gianlucadecola Apr 12, 2022
a6a7cd9
remove unused main code.
gianlucadecola Apr 14, 2022
42b5ead
Adding type hints.
gianlucadecola Apr 15, 2022
7dabd13
catch custom exception in tests.
gianlucadecola Apr 15, 2022
97fa4e9
Fix magic numbers.
gianlucadecola Apr 15, 2022
808d0a1
Add test with an actual environment.
gianlucadecola Apr 15, 2022
bf4d2d8
fix comment.
gianlucadecola Apr 15, 2022
52b37cc
Add TODO memo on env.render.
gianlucadecola Apr 15, 2022
5a8f6a4
change map with list comprehension.
gianlucadecola Apr 15, 2022
b5aae1b
remove unused imports.
gianlucadecola Apr 15, 2022
e9d333b
Add type hint.
gianlucadecola Apr 16, 2022
b0081c2
typo.
gianlucadecola Apr 16, 2022
a02323c
docstring.
gianlucadecola Apr 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add type hint.
  • Loading branch information
@gianlucadecola
gianlucadecola committed Apr 11, 2022
commit 48d5f07f21331bcf08cf7e8cf05e39cd6d577207
12 changes: 7 additions & 5 deletions gym/utils/play.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import argparse
from typing import Tuple

import pygame
from pygame.event import Event

import gym
from gym import logger
from gym import Env, logger

try:
import matplotlib
Expand All @@ -20,15 +22,15 @@


class PlayableGame:
def __init__(self, env, keys_to_action=None, zoom=None):
def __init__(self, env: Env, keys_to_action: dict = None, zoom: float = None):
Copy link
Contributor

@RedTachyon RedTachyon Apr 14, 2022

Choose a reason for hiding this comment

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

keys_to_action: Optional[dict]

Copy link
Contributor

@RedTachyon RedTachyon Apr 14, 2022

Choose a reason for hiding this comment

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

Even better if we can specify the key/value types on the dict (I guess values are arbitrary actions, but keys should be keypresses? how are they represented?)

Copy link
Contributor Author

@gianlucadecola gianlucadecola Apr 14, 2022

Choose a reason for hiding this comment

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

It's Dict[Tuple[int], int] where the key is (ord(key),)

gym/gym/envs/classic_control/mountain_car.py

Lines 240 to 242 in 5ae6bf9

def get_keys_to_action(self):
# Control with left and right arrow keys.
return {(): 1, (276,): 0, (275,): 2, (275, 276): 1}

reference of the actions:

gym/gym/envs/classic_control/mountain_car.py

Lines 50 to 54 in 5ae6bf9

| Num | Observation | Value | Unit |
|-----|-------------------------------------------------------------|---------|------|
| 0 | Accelerate to the left | Inf | position (m) |
| 1 | Don't accelerate | Inf | position (m) |
| 2 | Accelerate to the right | Inf | position (m) |

gym/gym/utils/play.py

Lines 125 to 135 in 9a1d4b6

keys_to_action: dict: tuple(int) -> int or None
Mapping from keys pressed to action performed.
For example if pressed 'w' and space at the same time is supposed
to trigger action number 2 then key_to_action dict would look like this:
{
# ...
sorted(ord('w'), ord(' ')) -> 2
# ...
}
If None, default key_to_action mapping for that env is used, if provided.

self.env = env
self.relevant_keys = self._get_relevant_keys(keys_to_action)
self.video_size = self._get_video_size(zoom)
self.screen = pygame.display.set_mode(self.video_size)
self.pressed_keys = []
self.running = True

def _get_relevant_keys(self, keys_to_action):
def _get_relevant_keys(self, keys_to_action: dict) -> set:
Copy link
Contributor

@RedTachyon RedTachyon Apr 14, 2022

Choose a reason for hiding this comment

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

Optional[dict]

if keys_to_action is None:
if hasattr(self.env, "get_keys_to_action"):
keys_to_action = self.env.get_keys_to_action()
Expand All @@ -43,7 +45,7 @@ def _get_relevant_keys(self, keys_to_action):
relevant_keys = set(sum(map(list, keys_to_action.keys()), []))
Copy link
Contributor

@RedTachyon RedTachyon Apr 14, 2022

Choose a reason for hiding this comment

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

map is nice (functional programming is great), but in Python it's typically more readable to use a comprehension, so it'd be something like
set(sum(list(key) for key in keys_to_action), []))

But is this actually what we want this to do? I'm not sure what each key is meant to be (see previous comment about types), so take another look at the logic here. Intuition tells me that you might have wanted to just do set(keys_to_actions.keys()) or something like that (which might be equivalent to just set(keys_to_actions))

Copy link
Contributor Author

@gianlucadecola gianlucadecola Apr 14, 2022

Choose a reason for hiding this comment

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

I'm going to explore it more in depth, this logic was already there in the old play(), it seems also to me that set(keys_to_actions.keys()) may be fine

return relevant_keys

def _get_video_size(self, zoom=None):
def _get_video_size(self, zoom: float = None) -> Tuple[int, int]:
Copy link
Contributor

@RedTachyon RedTachyon Apr 14, 2022

Choose a reason for hiding this comment

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

Optional[float]

rendered = self.env.render(mode="rgb_array")
Copy link
Contributor

@RedTachyon RedTachyon Apr 14, 2022

Choose a reason for hiding this comment

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

Can you put like a TODO here so that we remember to update this when the render API change goes through?

video_size = [rendered.shape[1], rendered.shape[0]]

Expand All @@ -52,7 +54,7 @@ def _get_video_size(self, zoom=None):

return video_size

def process_event(self, event):
def process_event(self, event: Event) -> None:
if event.type == pygame.KEYDOWN:
if event.key in self.relevant_keys:
self.pressed_keys.append(event.key)
Expand Down