Repository: sail-sg/envpool (language: python)

Issue:
Atari option for repeat_action_probability
The -v5 Gym Atari environments have sticky actions enabled by default (with `repeat_action_probability=0.25`, see [here](https://github.com/mgbellemare/Arcade-Learning-Environment#openai-gym)). This makes it impossible to replicate the original results from several key papers, especially the DQN Nature paper.

Would it be possible to add an option to [the Atari environment options](https://envpool.readthedocs.io/en/latest/api/atari.html#options) that lets the user change `repeat_action_probability` to a different value? I believe that internally this can be accomplished by forwarding the argument to either `gym.make` or the ALE constructor.



Discussion / hints:
I think there may be some misunderstanding:

In *NoFrameskip-v4 setting with openai wrappers:
https://github.com/openai/gym/blob/0.18.3/gym/envs/__init__.py#L656
and https://github.com/openai/gym/blob/0.18.3/gym/envs/atari/atari_env.py#L33
You can see the default value of repeat_action_probability is set to 0.

Currently, EnvPool's atari implementation follows the openai wrappers (that is to say we follow \*NoFrameskip-v4 setting) instead of the latest gym version (\*-v5), and as you can see https://github.com/sail-sg/envpool/blob/c6fd55dc61e07e5e547623711ab35d6ac8af06ed/envpool/atari/atari_env.h#L115

> Would it be possible to add an option to the Atari environment options that lets the user change repeat_action_probability to a different value?

Sure, will do.
Oh awesome! I didn't see that the default value is 0, but it would still be good to have an option to change it. Thanks for looking into it.
And also here: https://github.com/mgbellemare/Arcade-Learning-Environment/blob/master/src/python/gym.py#L148 it is set to 0
Indeed. My confusion was because the environment names in your documentation (https://envpool.readthedocs.io/en/latest/api/atari.html#available-tasks) are called *-v5, so I assumed that they followed the defaults for Gym *-v5 instead of *NoFrameSkip-v4. Thank you for clarifying.
That's indeed an issue. Do you have any better idea instead of posting something like "our implementation is not equal to gym's *-v5" on the top of Atari env API documentation?
Maybe you could just drop the Gym-like versioning and just call them Adventure, AirRaid, etc?

Of course, that would mean that you can't change the default arguments in future releases (without unexpectedly changing behavior). However, since people can change the values themselves anyway, I don't think that's a huge issue. For example, if I do `envpool.make('Pong-v5', env_type='gym', repeat_action_probability=0.9)`, then it's already not equivalent to Pong-v5, so the name didn't matter in the first place.

If you are concerned about future compatibility though, then maybe you could have your own naming convention that doesn't clash with Gym. Something like AdventureEnvPool-v0, etc. where it's clear that your versions aren't related to Gym.

Fix patch:
diff --git a/docs/api/atari.rst b/docs/api/atari.rst
index dcc24562..8865c440 100644
--- a/docs/api/atari.rst
+++ b/docs/api/atari.rst
@@ -1,8 +1,8 @@
 Atari
 =====
 
-We use ``ale_py>=0.7.2`` as the codebase.
-See https://github.com/mgbellemare/Arcade-Learning-Environment/tree/v0.7.2
+We use ``ale_py==0.7.3`` as the codebase.
+See https://github.com/mgbellemare/Arcade-Learning-Environment/tree/v0.7.3
 
 
 Env Wrappers
@@ -42,7 +42,10 @@ Options
   behavior (since there is no ``discount`` field in gym's API). Default to
   ``False``;
 * ``reward_clip (bool)``: whether to change the reward to ``sign(reward)``,
-  default to ``False``.
+  default to ``False``;
+* ``repeat_action_probability (float)``: the action repeat probability in ALE
+  configuration, default to 0 (no action repeat to perform deterministic
+  result).
 
 Observation Space
 -----------------
@@ -61,6 +64,11 @@ Each Atari games has its own discrete action space.
 Available Tasks
 ---------------
 
+**Note: Our Atari environments ALE settings follow gym's** ``*NoFrameSkip-v4``
+(with openai/baselines wrapper) **instead of** ``*-v5`` by default, see the
+related discussions at
+`Issue #14 <https://github.com/sail-sg/envpool/issues/14>`_.
+
 * ``Adventure-v5``
 * ``AirRaid-v5``
 * ``Alien-v5``
diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt
index 5ceb8987..3b9ebb6b 100644
--- a/docs/spelling_wordlist.txt
+++ b/docs/spelling_wordlist.txt
@@ -19,3 +19,4 @@ parallelization
 parallelly
 mypy
 numpy
+openai
diff --git a/envpool/atari/atari_env.h b/envpool/atari/atari_env.h
index 5e066bef..17d8cb45 100644
--- a/envpool/atari/atari_env.h
+++ b/envpool/atari/atari_env.h
@@ -54,7 +54,8 @@ class AtariEnvFns {
                     "zero_discount_on_life_loss"_.bind(false),
                     "episodic_life"_.bind(false), "reward_clip"_.bind(false),
                     "img_height"_.bind(84), "img_width"_.bind(84),
-                    "task"_.bind(std::string("pong")));
+                    "task"_.bind(std::string("pong")),
+                    "repeat_action_probability"_.bind(0.0f));
   }
   template <typename Config>
   static decltype(auto) StateSpec(const Config& conf) {
@@ -112,7 +113,8 @@ class AtariEnv : public Env<AtariEnvSpec> {
         episodic_life_(spec.config["episodic_life"_]),
         done_(true),
         dist_noop_(0, spec.config["noop_max"_] - 1) {
-    env_->setFloat("repeat_action_probability", 0);
+    env_->setFloat("repeat_action_probability",
+                   spec.config["repeat_action_probability"_]);
     env_->setInt("random_seed", seed_);
     env_->loadROM(GetRomPath(spec.config["base_path"_], spec.config["task"_]));
     action_set_ = env_->getMinimalActionSet();
@@ -134,15 +136,17 @@ class AtariEnv : public Env<AtariEnvSpec> {
 
   void Reset() override {
     int noop = dist_noop_(gen_) + 1 - fire_reset_;
+    int push_num = 1;
     if (env_->game_over() || elapsed_step_ >= max_episode_steps_) {
       env_->reset_game();
       elapsed_step_ = 0;
-      ResetObsBuffer();
+      push_num = stack_num_;
     }
     while (noop--) {
       env_->act((ale::Action)0);
       if (env_->game_over()) {
         env_->reset_game();
+        push_num = stack_num_;
       }
     }
     if (fire_reset_) {
@@ -152,7 +156,9 @@ class AtariEnv : public Env<AtariEnvSpec> {
     uint8_t* ptr = static_cast<uint8_t*>(maxpool_buf_[0].data());
     env_->theOSystem->colourPalette().applyPaletteRGB(ptr, ale_screen_data,
                                                       kRawSize);
-    PushStack();
+    for (int i = 0; i < push_num; ++i) {
+      PushStack();
+    }
     done_ = false;
     State state = Allocate();
     state["discount"_] = 1.0f;
diff --git a/envpool/workspace0.bzl b/envpool/workspace0.bzl
index e4022118..a77ded45 100644
--- a/envpool/workspace0.bzl
+++ b/envpool/workspace0.bzl
@@ -141,10 +141,10 @@ def workspace():
     maybe(
         http_archive,
         name = "ale",
-        sha256 = "b2b9c33292ea3386af61371d94ab0c5d07c61010754f0dc4447f0b14b71303e9",
-        strip_prefix = "Arcade-Learning-Environment-0.7.2",
+        sha256 = "e3bada34cc6c116377c4a807c24d9890ce33afa854ffc45e32dc90ba0dcc9140",
+        strip_prefix = "Arcade-Learning-Environment-0.7.3",
         urls = [
-            "https://github.com/mgbellemare/Arcade-Learning-Environment/archive/refs/tags/v0.7.2.tar.gz",
+            "https://github.com/mgbellemare/Arcade-Learning-Environment/archive/refs/tags/v0.7.3.tar.gz",
         ],
         build_file = "//third_party/ale:ale.BUILD",
     )


Test patch:
diff --git a/envpool/atari/atari_env_test.cc b/envpool/atari/atari_env_test.cc
index f3ddf42a..a7bad9f1 100644
--- a/envpool/atari/atari_env_test.cc
+++ b/envpool/atari/atari_env_test.cc
@@ -51,12 +51,16 @@ TEST(AtariEnvTest, Seed) {
               std::vector<std::size_t>({batch, 4, 84, 84}));
     uint8_t* data0 = static_cast<uint8_t*>(state0["obs"_].data());
     uint8_t* data1 = static_cast<uint8_t*>(state1["obs"_].data());
-    int sum = 0;
-    for (std::size_t j = 0; j < batch * 4 * 84 * 84; ++j) {
-      EXPECT_EQ(data0[j], data1[j]);
-      sum += data0[j];
+    int index = 0;
+    for (std::size_t j = 0; j < batch * 4; ++j) {
+      // ensure there's no black screen in each frame
+      int sum = 0;
+      for (int k = 0; k < 84 * 84; ++k) {
+        EXPECT_EQ(data0[index], data1[index]);
+        sum += data0[index++];
+      }
+      EXPECT_NE(sum, 0) << i << " " << j;
     }
-    EXPECT_NE(sum, 0) << i;
     action["env_id"_] = state0["info:env_id"_];
     action["players.env_id"_] = state0["info:env_id"_];
     action["action"_] = Array(Spec<int>({static_cast<int>(batch)}));
@@ -76,6 +80,7 @@ TEST(AtariEnvTest, MaxEpisodeSteps) {
   config["batch_size"_] = batch;
   config["seed"_] = 0;
   config["max_episode_steps"_] = max_episode_steps;
+  config["repeat_action_probability"_] = 0.2f;
   int total_iter = 100;
   atari::AtariEnvSpec spec(config);
   atari::AtariEnvPool envpool(spec);