From efb97f667f18bd048ffbc21ea771799ce83449d5 Mon Sep 17 00:00:00 2001
From: u214892 <u214892@sbb.ch>
Date: Wed, 13 Nov 2019 08:39:53 -0500
Subject: [PATCH] SIM-119 refactoring review from Christian B.
---
flatland/action_plan/action_plan.py | 12 ++++++------
flatland/envs/rail_env_shortest_paths.py | 9 ++++++++-
flatland/envs/rail_train_run_data_structures.py | 2 +-
tests/test_action_plan.py | 2 +-
4 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/flatland/action_plan/action_plan.py b/flatland/action_plan/action_plan.py
index 59c4f1f3..5edad878 100644
--- a/flatland/action_plan/action_plan.py
+++ b/flatland/action_plan/action_plan.py
@@ -18,7 +18,7 @@ ActionPlanElement = NamedTuple('ActionPlanElement', [
# an action plan gathers all the the actions to be taken by a single agent at the corresponding time steps
ActionPlan = List[ActionPlanElement]
-# An action plan dict gathers all the actions for every agent identified by the dictionary key
+# An action plan dict gathers all the actions for every agent identified by the dictionary key = agent_handle
ActionPlanDict = Dict[int, ActionPlan]
@@ -29,10 +29,10 @@ class DeterministicController():
env: RailEnv,
train_run_dict: Dict[int, TrainRun]):
- self.env = env
+ self.env: RailEnv = env
self.train_run_dict: Dict[int, TrainRun] = train_run_dict
- self.action_plan = [self._create_action_plan_for_agent(agent_id, chosen_path)
- for agent_id, chosen_path in train_run_dict.items()]
+ self.action_plan: ActionPlanDict = [self._create_action_plan_for_agent(agent_id, chosen_path)
+ for agent_id, chosen_path in train_run_dict.items()]
def get_way_point_before_or_at_step(self, agent_id: int, step: int) -> WayPoint:
"""
@@ -84,7 +84,6 @@ class DeterministicController():
"""
for action_plan_element in self.action_plan[agent_id]:
- action_plan_element: ActionPlanElement = action_plan_element
scheduled_at = action_plan_element.scheduled_at
if scheduled_at > current_step:
return None
@@ -95,6 +94,7 @@ class DeterministicController():
def act(self, current_step: int) -> Dict[int, RailEnvActions]:
"""
Get the action dictionary to be replayed at the current step.
+ Returns only action where required (no action for done agents or those not at the beginning of the cell).
Parameters
----------
@@ -119,7 +119,7 @@ class DeterministicController():
print(" {}".format(step))
@staticmethod
- def compare_action_plans(expected_action_plan: ActionPlanDict, actual_action_plan: ActionPlanDict):
+ def assert_actions_plans_equal(expected_action_plan: ActionPlanDict, actual_action_plan: ActionPlanDict):
assert len(expected_action_plan) == len(actual_action_plan)
for k in range(len(expected_action_plan)):
assert len(expected_action_plan[k]) == len(actual_action_plan[k]), \
diff --git a/flatland/envs/rail_env_shortest_paths.py b/flatland/envs/rail_env_shortest_paths.py
index d5f91cb0..275aadc7 100644
--- a/flatland/envs/rail_env_shortest_paths.py
+++ b/flatland/envs/rail_env_shortest_paths.py
@@ -20,6 +20,9 @@ def get_valid_move_actions_(agent_direction: Grid4TransitionsEnum,
"""
Get the valid move actions (forward, left, right) for an agent.
+ TODO https://gitlab.aicrowd.com/flatland/flatland/issues/299 The implementation could probably be more efficient
+ and more elegant. But given the few calls this has no priority now.
+
Parameters
----------
agent_direction : Grid4TransitionsEnum
@@ -76,6 +79,9 @@ def get_new_position_for_action(
"""
Get the next position for this action.
+ TODO https://gitlab.aicrowd.com/flatland/flatland/issues/299 The implementation could probably be more efficient
+ and more elegant. But given the few calls this has no priority now.
+
Parameters
----------
agent_position
@@ -137,7 +143,8 @@ def get_action_for_move(
"""
Get the action (if any) to move from a position and direction to another.
- The implementation could probably be more efficient I believe. But given the few calls this has no priority now.
+ TODO https://gitlab.aicrowd.com/flatland/flatland/issues/299 The implementation could probably be more efficient
+ and more elegant. But given the few calls this has no priority now.
Parameters
----------
diff --git a/flatland/envs/rail_train_run_data_structures.py b/flatland/envs/rail_train_run_data_structures.py
index aeb34a00..016862f1 100644
--- a/flatland/envs/rail_train_run_data_structures.py
+++ b/flatland/envs/rail_train_run_data_structures.py
@@ -14,5 +14,5 @@ TrainRunWayPoint = NamedTuple('TrainRunWayPoint', [
('scheduled_at', int),
('way_point', WayPoint)
])
-# A path schedule is the list of an agent's cell pin entries
+# A train run is the list of an agent's way points and their scheduled time
TrainRun = List[TrainRunWayPoint]
diff --git a/tests/test_action_plan.py b/tests/test_action_plan.py
index 323d653f..c6176689 100644
--- a/tests/test_action_plan.py
+++ b/tests/test_action_plan.py
@@ -84,5 +84,5 @@ def test_action_plan(rendering: bool = False):
deterministic_controller = DeterministicController(env, chosen_path_dict)
deterministic_controller.print_action_plan()
- DeterministicController.compare_action_plans(expected_action_plan, deterministic_controller.action_plan)
+ DeterministicController.assert_actions_plans_equal(expected_action_plan, deterministic_controller.action_plan)
DeterministicControllerReplayer.replay_verify(MAX_EPISODE_STEPS, deterministic_controller, env, rendering)
--
GitLab