box.contains check dtype and promote non-ndarrays

[FirefoxMetzger]

Closes: #2357 and #2298

Instead of only casting list to ndarray, cast any class to ndarray (if possible) and emit a warning when casting. Also, check if the dtype of the input matches the dtype of the space.

[FirefoxMetzger]

@tristandeleu Looks like gym's tests depend on Box.contains old behavior of not checking types (https://github.com/openai/gym/pull/2374/checks?check_run_id=3450441531#step:4:252). Potentially downstream code will do this, too, making the current PR a breaking change. What is gym's approach to such scenarios? Change the function to make the dtype check optional and backward-compatible (i.e. add a enforce_dtype_match kwarg or similar) or modify the existing tests since this is a bugfix?

@jkterry1 Since the current version of the PR would contain a breaking change (due to the additional check suggested by #2298 ) do you have an opinion/preference on this matter?

[tristandeleu]

There are two issues coming out of those tests:

• KellyCoinflipEnv is returning an observation without specifying the dtype, so it falls back to np.float64 by default even though the observation_space has a dtype of np.float32. I think this fix would be reasonable in that case (and it might not even matter too much since this environment might get moved out of Gym [Proposal] Moving uncommonly used toy text environments from Gym into different repo #2369):

diff --git a/gym/envs/toy_text/kellycoinflip.py b/gym/envs/toy_text/kellycoinflip.py
index 1a47c0a..4f305ab 100644
--- a/gym/envs/toy_text/kellycoinflip.py
+++ b/gym/envs/toy_text/kellycoinflip.py
@@ -79,7 +79,7 @@ class KellyCoinflipEnv(gym.Env):
         return self._get_obs(), reward, done, {}

     def _get_obs(self):
-        return np.array([self.wealth]), self.rounds
+        return np.array([self.wealth], dtype=np.float32), self.rounds

     def reset(self):
         self.rounds = self.max_rounds
@@ -236,11 +236,11 @@ class KellyCoinflipGeneralizedEnv(gym.Env):

     def _get_obs(self):
         return (
-            np.array([float(self.wealth)]),
+            np.array([float(self.wealth)], dtype=np.float32),
             self.rounds_elapsed,
             self.wins,
             self.losses,
-            np.array([float(self.max_ever_wealth)]),
+            np.array([float(self.max_ever_wealth)], dtype=np.float32),
         )

     def reset(self):

• The tests for the FlattenObservation wrapper are failing because of an invalid dtype for the space. This is something I missed in Fix flatten utilities for spaces #2328. Flattening a Tuple of Discrete spaces leads to a Box spaces corresponding to multi-hot encoding (with dtype np.int64), and flattening a Tuple of Box and Discrete has to have a dtype of np.float64 (dtype resolved with np.result_type). I would suggest to fix those tests to reflect that (this is not a problem with this PR, on the contrary it highlighted an incorrect test!).

diff --git a/gym/wrappers/test_flatten_observation.py b/gym/wrappers/test_flatten_observation.py
index f190081..2db176c 100644
--- a/gym/wrappers/test_flatten_observation.py
+++ b/gym/wrappers/test_flatten_observation.py
@@ -19,12 +19,12 @@ def test_flatten_observation(env_id):
         space = spaces.Tuple(
             (spaces.Discrete(32), spaces.Discrete(11), spaces.Discrete(2))
         )
-        wrapped_space = spaces.Box(-np.inf, np.inf, [32 + 11 + 2], dtype=np.float32)
+        wrapped_space = spaces.Box(0, 1, [32 + 11 + 2], dtype=np.int64)
     elif env_id == "KellyCoinflip-v0":
         space = spaces.Tuple(
             (spaces.Box(0, 250.0, [1], dtype=np.float32), spaces.Discrete(300 + 1))
         )
-        wrapped_space = spaces.Box(-np.inf, np.inf, [1 + (300 + 1)], dtype=np.float32)
+        wrapped_space = spaces.Box(-np.inf, np.inf, [1 + (300 + 1)], dtype=np.float64)

     assert space.contains(obs)
     assert wrapped_space.contains(wrapped_obs)

[FirefoxMetzger]

@tristandeleu I updated the tests; are we worried about informing downstream about this change? I.e. should we bump a version, update a changelog, or leave a comment somewhere saying "Breaking change: Box.contains now also matches dtype when checking for membership" or something?

[tristandeleu]

@FirefoxMetzger Yes this will probably require bumping the version, to avoid any surprises. I don't know what is the status on the changelog though (there was a discussion here #2275, but I don't know what was the outcome), but it would definitely be a good idea to inform users of this change through the changelog.

[jkterry1]

This was stated elsewhere, but the changelogs are going in GitHub release notes, e.g. here is the changelog for the last release: https://github.com/openai/gym/releases/tag/0.19.0

[jkterry1]

@FirefoxMetzger this needs tests

[FirefoxMetzger]

@jkterry1 I've added a unit test to avoid regression regarding the two issues addressed by this PR. Also note that this is also covered by existing tests (the two that were adjusted as part of this PR).

[jkterry1]

Just to confirm, the only version bump that would be required would be for KellyCoinflip right?

[FirefoxMetzger]

@jkterry1 Good catch with the environment version. I'm not an active user of KellyCoinflip, so I can't estimate if the reduction in precision (float64 -> float32) will affect performance or existing algorithms. If so, then the version of the environment should indeed be bumped.

The version bump I was talking about with @tristandeleu was related to the overall gym version. The function Box.contains behaves differently/more strictly now (it now checks if the provided object has the same dtype as the space it represents). As such, it may break existing code outside of gym that (explicitly or implicitly) relies on Box.contains not enforcing the dtype. Hence the question about bumping versions or how to communicate this best.

[rohanb2018]

I have sort of an unrelated comment, but I noticed in Box.contains that the np.all clauses were changed to np.any clauses. Was there a particular reason for this change? I feel like np.all is the correct logic for Box.contains, because we need all of the coordinates of the test point x to satisfy the lower/upper bounds, not just any of them.

[FirefoxMetzger]

@rohanb2018 Good catch; this is not how it should be. It changed from np.all(x >= self.low) to not np.any(x < self.low) and then back to np.any(x >= self.low) which is of course not correct. I'll flip the signs in a new PR later today.