Maze/AntMaze issues #156

alexdavey · 2023-06-20T22:50:05Z

Description

Fixes #155

refactored compute_terminated so it is now a pure function, added update_goal
added info["success"] key to AntMaze
fixed sparse reward issue in AntMaze
fixed reset issue in Maze
added test for reset issue
bumped version to AntMaze-v4

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

rodrigodelazcano · 2023-06-20T23:04:56Z

@alexdavey. We should be keeping previous versions of the environment such as in Gymnasium. You can create different file versions for each environment version antmaze_v3.py and antmaze_v4.py. The updates in maze.py don't have any breaking changes so it doesn't require a version bump.

tests/envs/ant_maze/test_reset.py

rodrigodelazcano · 2023-06-20T23:52:44Z

@alexdavey I think the test is failing because the naming test_reset.py can't be repeated.

Actually, I would prefer if the test files have the same structure as the Gymnasium repo. Add two files tests/envs/maze/point_maze.py and tests/envs/maze/ant_maze.py where we can add tests to each environment.

The reward is now calculated before the goal is updated in compute_terminated(), as in point_maze. Important for e.g. sparse rewards.

Reset/goal distance threshold now scales with maze size. Prevents AntMaze resetting into a goal position due to added noise. Added test that now passes.

gymnasium_robotics/envs/maze/ant_maze.py

Kallinteris-Andreas · 2023-06-21T10:10:52Z

gymnasium_robotics/envs/maze/ant_maze.py


        return obs_dict, info

    def step(self, action):
        ant_obs, _, _, _, info = self.ant_env.step(action)
        obs = self._get_obs(ant_obs)

+        info["success"] = bool(np.linalg.norm(obs["achieved_goal"] - self.goal) <= 0.45)


info should be after truncated

I think the order doesn't matter. Correct me if I'm missing anything @Kallinteris-Andreas

It does not affect the behavior, but makes it more readable

Sure. @alexdavey if you can update it as per @Kallinteris-Andreas request that'll be great :)

gymnasium_robotics/envs/maze/maze.py

Kallinteris-Andreas · 2023-06-21T10:13:44Z

gymnasium_robotics/envs/maze/maze.py

+            # Generate another goal
+            goal = self.generate_target_goal()
+            # Add noise to goal position
+            self.goal = self.add_xy_position_noise(goal)


Do we need to test if the object I near the goal (0.45)

I agree. @alexdavey can you add a while loop to keep generating a goal until the distance to achieved_goal is greater than the 0.45 threshold?

gymnasium_robotics/envs/maze/ant_maze.py

gymnasium_robotics/envs/maze/maze.py

alexdavey · 2023-06-21T20:55:16Z

@alexdavey. We should be keeping previous versions of the environment such as in Gymnasium. You can create different file versions for each environment version antmaze_v3.py and antmaze_v4.py. The updates in maze.py don't have any breaking changes so it doesn't require a version bump.

@rodrigodelazcano. I would have thought that themaze_size_scaling bug fix inmaze.py is a breaking change, since it will change the reset behaviour of the existing AntMaze version?

Edit: I have listed what seem like the most sensible version changes to me, below. Let me know if you are happy with these and I will update the PR accordingly:

maze.py: The original pre-PR file.
ant_maze.py: The original pre-PR file.
maze_v4.py: Includes refactor of compute_terminated() and the fix for the maze_size_scaling bug.
ant_maze_v4.py: Uses maze_v4.py and fixes sparse reward bug.
point_maze.py: Uses maze_v4.py but no other changes. No version bump as there should be no behaviour change.

rodrigodelazcano · 2023-06-22T00:05:10Z

@alexdavey. We should be keeping previous versions of the environment such as in Gymnasium. You can create different file versions for each environment version antmaze_v3.py and antmaze_v4.py. The updates in maze.py don't have any breaking changes so it doesn't require a version bump.

@rodrigodelazcano. I would have thought that themaze_size_scaling bug fix inmaze.py is a breaking change, since it will change the reset behaviour of the existing AntMaze version?

My take on this is that the maze_size_scaling is 1 so it won't affect the registered environments

@alexdavey. We should be keeping previous versions of the environment such as in Gymnasium. You can create different file versions for each environment version antmaze_v3.py and antmaze_v4.py. The updates in maze.py don't have any breaking changes so it doesn't require a version bump.

@rodrigodelazcano. I would have thought that themaze_size_scaling bug fix inmaze.py is a breaking change, since it will change the reset behaviour of the existing AntMaze version?

Edit: I have listed what seem like the most sensible version changes to me, below. Let me know if you are happy with these and I will update the PR accordingly:
* `maze.py`: The original pre-PR file.

* `ant_maze.py`: The original pre-PR file.

* `maze_v4.py`: Includes refactor of `compute_terminated()` and the fix for the `maze_size_scaling` bug.

* `ant_maze_v4.py`: Uses `maze_v4.py` and fixes sparse reward bug.

* `point_maze.py`: Uses `maze_v4.py` but no other changes. No version bump as there should be no behaviour change.

Sounds good. My take on maze.py is that maze_size_scaling has a value of 1 so it wouldn't affect the registered envs unless you are modifying this argument. However, I agree lets bump to be sure.

rodrigodelazcano

I agree with the version changes. I also added some minor comments on @Kallinteris-Andreas review and other updates for the PR., Thank you @alexdavey

tests/envs/maze/test_ant_maze.py

tests/envs/maze/test_point_maze.py

alexdavey · 2023-06-22T00:12:13Z

Sounds good. My take on maze.py is that maze_size_scaling has a value of 1 so it wouldn't affect the registered envs unless you are modifying this argument. However, I agree lets bump to be sure.

In AntMaze, the MazeEnv parent is initalised with maze_size_scaling = 4, so I believe that AntMaze would change behaviour with the new maze.py changes, but do let me know if I am mistaken.

Thanks to both of you for the reviews.

rodrigodelazcano · 2023-06-22T00:15:25Z

Sounds good. My take on maze.py is that maze_size_scaling has a value of 1 so it wouldn't affect the registered envs unless you are modifying this argument. However, I agree lets bump to be sure.

In AntMaze, the MazeEnv parent is initalised with maze_size_scaling = 4, so I believe that AntMaze would change behaviour with the new maze.py changes, but do let me know if I am mistaken.

Thanks to both of you for the reviews.

You are totally right! Then I take my words back, bumping makes more sense

* restored ant_maze.py from pre-PR state * restored maze.py from pre-PR state * add test for info['success'] = false on reset * add distance check when generating new goal in update_goal()

rodrigodelazcano · 2023-06-22T20:58:56Z

gymnasium_robotics/envs/maze/maze.py

@@ -40,6 +40,9 @@ class Maze:
    The Maze class also presents a method to convert from cell indices to `(x,y)` coordinates in the MuJoCo simulation:
    - :meth:`cell_rowcol_to_xy` - Convert from `(i,j)` to `(x,y)`

+    ### Version History


@alexdavey Can you remove the docstrings for the versions in ant_maze.py and maze.py?

I will remove the ones I added to maze.py. I think ant_maze.py has no changes currently from the existing version -- did you want me to remove them anyway?

As a standard we only keep docstrings in the latest file version. That'll be ant_maze_v4.py and maze_v4.py. So yes remove them for both maze,.py and ant_maze.py

Makes sense, will do now

rodrigodelazcano · 2023-06-22T21:21:12Z

Thanks a lot @alexdavey . I'll merge this after the CI checks

alexdavey · 2023-06-22T21:22:46Z

I just removed some final docstrings in maze.py that were missed initially. I think all issues have been resolved as well. Thanks.

rodrigodelazcano requested changes Jun 20, 2023

View reviewed changes

tests/envs/ant_maze/test_reset.py Outdated Show resolved Hide resolved

tests/envs/ant_maze/test_reset.py Outdated Show resolved Hide resolved

alexdavey added 11 commits June 21, 2023 01:22

Add info success key

b5734e0

Move AntMaze reward before term/trunc

276e81e

The reward is now calculated before the goal is updated in compute_terminated(), as in point_maze. Important for e.g. sparse rewards.

Prevent AntMaze resetting into a goal position

fcb6180

Reset/goal distance threshold now scales with maze size. Prevents AntMaze resetting into a goal position due to added noise. Added test that now passes.

pre-commit/Black modifications

2238562

Refactor compute_terminated

1f2cf5b

Update to AntMaze_UMaze-v4 etc

86c5b45

Update documentation

263ecd4

Fixed/added tests and env versions

6285458

Fixed conflicting test names

4805bea

Rearranged AntMaze versions for consistency

7af2b33

Changed test file structure to match Gymnasium

ac40967

alexdavey force-pushed the antmaze branch from 3ae2b65 to ac40967 Compare June 21, 2023 00:24

alexdavey requested a review from rodrigodelazcano June 21, 2023 00:31

Kallinteris-Andreas reviewed Jun 21, 2023

View reviewed changes

rodrigodelazcano reviewed Jun 22, 2023

View reviewed changes

tests/envs/maze/test_ant_maze.py Show resolved Hide resolved

tests/envs/maze/test_point_maze.py Show resolved Hide resolved

Fix AntMaze/Maze versioning

a3a9acd

* restored ant_maze.py from pre-PR state * restored maze.py from pre-PR state * add test for info['success'] = false on reset * add distance check when generating new goal in update_goal()

rodrigodelazcano approved these changes Jun 22, 2023

View reviewed changes

Removed docstrings for outdated envs

1252b03

Remove missed docstrings

c417816

rodrigodelazcano merged commit 3ab81e2 into Farama-Foundation:main Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maze/AntMaze issues #156

Maze/AntMaze issues #156

alexdavey commented Jun 20, 2023

rodrigodelazcano commented Jun 20, 2023 •

edited

Loading

rodrigodelazcano commented Jun 20, 2023

Kallinteris-Andreas Jun 21, 2023

rodrigodelazcano Jun 21, 2023

Kallinteris-Andreas Jun 22, 2023

rodrigodelazcano Jun 22, 2023

Kallinteris-Andreas Jun 21, 2023

rodrigodelazcano Jun 21, 2023

alexdavey commented Jun 21, 2023 •

edited

Loading

rodrigodelazcano commented Jun 22, 2023

rodrigodelazcano left a comment

alexdavey commented Jun 22, 2023 •

edited

Loading

rodrigodelazcano commented Jun 22, 2023

rodrigodelazcano Jun 22, 2023

alexdavey Jun 22, 2023

rodrigodelazcano Jun 22, 2023

alexdavey Jun 22, 2023

rodrigodelazcano commented Jun 22, 2023

alexdavey commented Jun 22, 2023

Maze/AntMaze issues #156

Maze/AntMaze issues #156

Conversation

alexdavey commented Jun 20, 2023

Description

Type of change

Checklist:

rodrigodelazcano commented Jun 20, 2023 • edited Loading

rodrigodelazcano commented Jun 20, 2023

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

alexdavey commented Jun 21, 2023 • edited Loading

rodrigodelazcano commented Jun 22, 2023

rodrigodelazcano left a comment

Choose a reason for hiding this comment

alexdavey commented Jun 22, 2023 • edited Loading

rodrigodelazcano commented Jun 22, 2023

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

rodrigodelazcano commented Jun 22, 2023

alexdavey commented Jun 22, 2023

rodrigodelazcano commented Jun 20, 2023 •

edited

Loading

alexdavey commented Jun 21, 2023 •

edited

Loading

alexdavey commented Jun 22, 2023 •

edited

Loading