Frame stack #142

jackyoung96 · 2022-03-27T06:18:02Z

frame_stack.py modifying (same as stablebaseline3)
Original: fill observation stack by zero observations
Modify: fill observation stack by the copy of the first observation

aec_mock_test modifying (Reflecting modified frame stack)

benblack769 · 2022-03-27T21:52:12Z

@jackyoung96 Is this meant as a replacement for #141 ? If so, can you close that PR?

benblack769 · 2022-03-27T21:56:37Z

This PR looks pretty good. I understand what it is supposed to do, and the changes are simple and minimal.

Just a couple of additional things to make this production-ready:

Could you fix the documentation here to reflect the new changes? https://github.com/Farama-Foundation/SuperSuit/blob/master/README.md?plain=1#L47
Also, can you bump frame_stack_v1 to frame_stack_v2 (a simple search and replace throughout the repository should do the trick).

jackyoung96 · 2022-03-27T22:29:22Z

@jackyoung96 Is this meant as a replacement for #141 ? If so, can you close that PR?

Yes it is. Please close it.

This PR looks pretty good. I understand what it is supposed to do, and the changes are simple and minimal.

Just a couple of additional things to make this production-ready:

Could you fix the documentation here to reflect the new changes? https://github.com/Farama-Foundation/SuperSuit/blob/master/README.md?plain=1#L47

Also, can you bump frame_stack_v1 to frame_stack_v2 (a simple search and replace throughout the repository should do the trick).

I will. Thanks!

jackyoung96 · 2022-03-27T22:38:10Z

Could you check it? Thank you!

jackyoung96 · 2022-03-28T18:11:19Z

I add the stack_dim0 flag for stack_frame wrapper.

Pytorch uses channel first order and Tensorflow uses channel last order, so we can choose where the stacked dimension locate by stack_dim0 flag.

jjshoots · 2022-04-19T14:30:09Z

Hi @jackyoung96, can you fix linting and clear the test fails? Thanks!

jackyoung96 · 2022-04-19T14:43:11Z

Hi @jackyoung96, can you fix linting and clear the test fails? Thanks!

Sure, I will ASAP

jackyoung96 · 2022-04-19T14:49:48Z

@benblack769 Sorry for the late commit for the lint test failure. I totally forgot about it. I finally push the final version!

jjshoots · 2022-04-19T14:57:33Z

@jackyoung96 Thanks, we will likely merge this once the Atari work is done :)

jackyoung96 · 2022-04-19T15:07:45Z

Oh... there are still issues.... sorry for this.

jackyoung96 · 2022-04-19T18:41:26Z

This commit should be worked!

jjshoots · 2022-05-02T17:20:40Z

Hey @benblack769, sorry to drag you in, but do you know anything about the above error? I can't quite understand what's the error either.

benblack769 · 2022-05-03T14:16:47Z

I belive it is just a flaky error that comes up from time to time. I think perhaps legal moves are just chosen by chance in Mahjog so the game doesn't end quickly enough for the test to pass in 1/1000 games or something. A bit silly.

benblack769 · 2022-05-03T14:17:00Z

Just try rerunning the test

jjshoots · 2022-05-04T13:01:26Z

Hi @jackyoung96, tests are still failing, do you think that can be fixed?

jjshoots · 2022-05-04T13:19:21Z

@BolunDai0216

BolunDai0216 · 2022-05-04T13:28:58Z

@BolunDai0216

I think the tests are failing partially because of this line:

SuperSuit/supersuit/generic_wrappers/frame_stack.py

Line 66 in dca236d

def reset(self):

When creating reset(), seed now needs to be added as an argument, i.e., def reset(seed=None):

SuperSuit/supersuit/generic_wrappers/frame_stack.py

Line 29 in 0d7e5e2

def reset(self, seed=None):

@jackyoung96 Would it be possible to change your reset() in frame_stack_v2 so that it matches frame_stack_v1 argument-wise and see if that solves the issue?

jackyoung96 · 2022-05-04T16:00:42Z

@BolunDai0216 I fixed it and commit. I hope it can solve the issue.

jjshoots

Overall looks pretty good to me, just some general comments about code cleanliness some minor improvement. Once the comments are addressed, I'll get someone else to review it before merging.

Thanks for your contribution!

jjshoots · 2022-05-05T11:54:08Z

supersuit/utils/agent_indicator.py

@@ -8,17 +8,15 @@ def change_obs_space(space, num_indicators):
    if isinstance(space, Box):
        ndims = len(space.shape)
        if ndims == 1:
-            pad_space = np.ones((num_indicators,), dtype=space.dtype)
+            pad_space = np.max(space.high) * np.ones((num_indicators,), dtype=space.dtype)


What is the reason for this change?

It unify the high and low range of the indicator channel with the original observation channels.
Because sometimes the values original observation channel are in range of [0,255] (Before normalized)

Is it possible to instead do np.min(space.high)? This is in the very rare but potentially possible case of space.high = [1, 255, 255], pad_space will then not be contained within the observation space.

You could implement it to check that all space.high are the same values np.all(space.high[0] == space.high), and if they aren't then raise a warning. Doing this within the wrapper function would incur overhead cost though, so I think it's better to put this check in the wrapper init, and then just do np.min(space.high) everywhere else.

Got it! Thanks

jjshoots · 2022-05-05T11:59:25Z

README.md

+`frame_stack_v1(env, num_frames=4, stack_dim0=False)` stacks the most recent frames. For vector games observed via plain vectors (1D arrays), the output is just concatenated to a longer 1D array. 2D or 3D arrays are stacked to be taller 3D arrays. Stacked dimension can be set to dim=0 (default dim=-1) by stack_dim0=True. At the start of the game, frames that don't yet exist are filled with 0s. `num_frames=1` is analogous to not using this function.
+
+`frame_stack_v2(env, num_frames=4, stack_dim0=False)` stacks the most recent frames. For vector games observed via plain vectors (1D arrays), the output is just concatenated to a longer 1D array. 2D or 3D arrays are stacked to be taller 3D arrays. Stacked dimension can be set to dim=0 (default dim=-1) by stack_dim0=True. At the start of the game, frames that don't yet exist are filled with the copies of the first frame. `num_frames=1` is analogous to not using this function.


While I understand the idea behind stack_dim0, could we instead make it accept a stack_dim argument, and then add an assert that makes sure it's not any other value other than 0 or -1. Right now, while I'm sure this stack_dim=0 implementation works, somewhere down the line if there is an API change that allows stacking in arbitrary dimensions, the arguments would be different and will just be slightly less elegant. So I think it's best to get it right the first time so we don't need to change the API later in the future.

Also, do keep arguments (stack_dim0=True) in a code line.

jjshoots · 2022-05-05T12:10:14Z

supersuit/utils/frame_stack.py


    return tile_shape, new_shape


-def stack_obs_space(obs_space, stack_size):
+def stack_obs_space(obs_space, stack_size, stack_dim0=False):
    """
    obs_space_dict: Dictionary of observations spaces of agents
    stack_size: Number of frames in the observation stack


Need a comment here describing what stack_dim0 (or stack_dim possibly) does.

jjshoots · 2022-05-05T12:14:39Z

README.md

+`frame_stack_v1(env, num_frames=4, stack_dim0=False)` stacks the most recent frames. For vector games observed via plain vectors (1D arrays), the output is just concatenated to a longer 1D array. 2D or 3D arrays are stacked to be taller 3D arrays. Stacked dimension can be set to dim=0 (default dim=-1) by stack_dim0=True. At the start of the game, frames that don't yet exist are filled with 0s. `num_frames=1` is analogous to not using this function.
+
+`frame_stack_v2(env, num_frames=4, stack_dim0=False)` stacks the most recent frames. For vector games observed via plain vectors (1D arrays), the output is just concatenated to a longer 1D array. 2D or 3D arrays are stacked to be taller 3D arrays. Stacked dimension can be set to dim=0 (default dim=-1) with `stack_dim0=True`. At the start of the game, frames that don't yet exist are filled with the copies of the first frame. `num_frames=1` is analogous to not using this function.


While I'm sure that it works, can we change stack_dim0 to just become stack_dim, and then assert it to be either 0 or -1? This way if there are new API changes in the future that allow for arbitrary stacking dimensions, the arguments between the new API and this one won't be any different.

Also, do keep arguments (like stack_dim0=True) inside code lines.

jjshoots · 2022-05-05T12:17:07Z

supersuit/utils/agent_indicator.py

            return new_obs
        elif ndims == 3 or ndims == 2:
            obs = obs if ndims == 3 else np.expand_dims(obs, 2)
            old_shaped3 = obs.shape[2]
            new_obs = np.pad(obs, [(0, 0), (0, 0), (0, num_indicators)])
-            new_obs[:, :, old_shaped3 + indicator_num] = 1.0
+            new_obs[:, :, old_shaped3 + indicator_num] = np.max(space.high)


What's the reason for this change as well?

Same reason with #142 (comment)

jjshoots · 2022-05-05T12:23:48Z

supersuit/generic_wrappers/frame_stack.py

+                for _ in range(stack_size):
+                    self.stack = stack_obs(
+                        self.stack,
+                        obs,
+                        self.old_obs_space,
+                        stack_size,
+                        stack_dim0
+                    )
+                self.reset_flag = False


Is a for loop necessary here? Is it not possible to simply do something like tile_shape * obs? This is just to make things run a little bit faster.

I'm ok if a completely new function was created just for this functionality as well.

Your right. Actually, this is how frame_stack_v0 was implemented before, so I left it as it is. If you want, we can modify it, but I think it makes the code (very x 3) little faster 😅

If it makes the code a lot less readable then we can just keep the original implementation, but if it's simple to implement, I think changing it would be better. Small optimizations like this add up a lot in the long run (especially for python). 😄

…into frame_stack

jjshoots · 2022-05-06T09:36:49Z

Hi @jackyoung96, tests are still failing, though I think it's just linting.

jackyoung96 added 2 commits March 18, 2022 00:45

frame_stack_v1 modify: same as gym.wrappers.FrameStack

fcd9eb6

frame_stack modifying (same as stablebaseline3)

e3c2e6e

aec_mock_test modifying (Reflecting modified frame stack)

Create frame_stack_v2 and add explanation in Readme

7348740

jackyoung96 added 2 commits March 29, 2022 03:06

modify frame_stack: stack_dim0 flag add

16c75d7

add explanation about stack_dim0 in Readme

6f0aaf2

jackyoung96 added 2 commits March 30, 2022 09:30

minor modify at stack_obs

5113ffc

lint test

9e09d05

jjshoots mentioned this pull request Apr 19, 2022

frame_stack_v1 modify: same as gym.wrappers.FrameStack #139

Closed

modify for lint test

fc63dd5

indicator modify

9fa7b0a

jjshoots and others added 2 commits May 3, 2022 15:23

Update README.md

8578550

Merge branch 'master' into frame_stack

dca236d

add seed at reset method

650641b

jackyoung96 and others added 2 commits May 5, 2022 04:31

indent

fc206a3

Update README.md

eab4736

jjshoots reviewed May 5, 2022

View reviewed changes

jackyoung96 added 2 commits May 6, 2022 00:51

stack_dim0 -> stack_dim

ab279e0

Merge branch 'frame_stack' of https://github.com/jackyoung96/SuperSuit …

ed93321

…into frame_stack

line

a78bd51

jjshoots merged commit 9439cb9 into Farama-Foundation:master May 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frame stack #142

Frame stack #142

jackyoung96 commented Mar 27, 2022

benblack769 commented Mar 27, 2022

benblack769 commented Mar 27, 2022

jackyoung96 commented Mar 27, 2022

jackyoung96 commented Mar 27, 2022

jackyoung96 commented Mar 28, 2022

jjshoots commented Apr 19, 2022

jackyoung96 commented Apr 19, 2022

jackyoung96 commented Apr 19, 2022

jjshoots commented Apr 19, 2022

jackyoung96 commented Apr 19, 2022

jackyoung96 commented Apr 19, 2022

jjshoots commented May 2, 2022

benblack769 commented May 3, 2022

benblack769 commented May 3, 2022

jjshoots commented May 4, 2022

jjshoots commented May 4, 2022

BolunDai0216 commented May 4, 2022 •

edited

jackyoung96 commented May 4, 2022

jjshoots left a comment

jjshoots May 5, 2022

jackyoung96 May 5, 2022

jjshoots May 5, 2022 •

edited

jackyoung96 May 5, 2022

jjshoots May 5, 2022

jjshoots May 5, 2022

jjshoots May 5, 2022

jjshoots May 5, 2022

jackyoung96 May 5, 2022

jjshoots May 5, 2022

jackyoung96 May 5, 2022

jjshoots May 5, 2022

jjshoots commented May 6, 2022

		`frame_stack_v1(env, num_frames=4, stack_dim0=False)` stacks the most recent frames. For vector games observed via plain vectors (1D arrays), the output is just concatenated to a longer 1D array. 2D or 3D arrays are stacked to be taller 3D arrays. Stacked dimension can be set to dim=0 (default dim=-1) by stack_dim0=True. At the start of the game, frames that don't yet exist are filled with 0s. `num_frames=1` is analogous to not using this function.

		`frame_stack_v2(env, num_frames=4, stack_dim0=False)` stacks the most recent frames. For vector games observed via plain vectors (1D arrays), the output is just concatenated to a longer 1D array. 2D or 3D arrays are stacked to be taller 3D arrays. Stacked dimension can be set to dim=0 (default dim=-1) by stack_dim0=True. At the start of the game, frames that don't yet exist are filled with the copies of the first frame. `num_frames=1` is analogous to not using this function.

Frame stack #142

Frame stack #142

Conversation

jackyoung96 commented Mar 27, 2022

benblack769 commented Mar 27, 2022

benblack769 commented Mar 27, 2022

jackyoung96 commented Mar 27, 2022

jackyoung96 commented Mar 27, 2022

jackyoung96 commented Mar 28, 2022

jjshoots commented Apr 19, 2022

jackyoung96 commented Apr 19, 2022

jackyoung96 commented Apr 19, 2022

jjshoots commented Apr 19, 2022

jackyoung96 commented Apr 19, 2022

jackyoung96 commented Apr 19, 2022

jjshoots commented May 2, 2022

benblack769 commented May 3, 2022

benblack769 commented May 3, 2022

jjshoots commented May 4, 2022

jjshoots commented May 4, 2022

BolunDai0216 commented May 4, 2022 • edited

jackyoung96 commented May 4, 2022

jjshoots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjshoots May 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjshoots commented May 6, 2022

BolunDai0216 commented May 4, 2022 •

edited

jjshoots May 5, 2022 •

edited