86 Improve documentation MotionCheck #87

chenkins · 2024-11-20T07:02:15Z

Changes

Improve documentation of MotionCheck Behaviour #86.

Related issues

Resolves #86

Checklist

Tests are included for relevant behavior changes.
Documentation is added in the docs folder for relevant behavior changes. If you made important user-facing
changes, describe them under the [Unreleased] tag in CHANGELOG.md.
New package dependencies are declared in the pyproject.toml file.
Requirement files have been updated by running tox -e requirements.
Code works with all supported Python versions (3.10, 3.11 and 3.12). Checks run with all three version and are
required to run successfully.
Code is formatted according to PEP 8 (an IDE like PyCharm can do this for you).
Technical guidelines listed in CONTRIBUTING.md are followed.

tests/test_agent_chains.py

flatland/envs/agent_chains.py

…e location.

… refactoring suggestion.

chenkins · 2024-11-22T22:25:58Z

flatland/envs/agent_chains.py

+            dPred: Set[Cell] = dPred
+
+            # if in blocked, it will not also be in a swap pred tree, so no need to worry about overwriting (outdegree always  <= 1!)
+            # TODO why not mark outside of the loop? The loop would then only need to go over nodes with indegree >2 not marked purple or red yet


@hagrid67 @aiAdrian is there a reason why svBlocked are not all marked red outsie of the loop?

yes I can't see any reason why we don't use some sort of bulk action to mark svBlocked as "red"; though it may not save much time as it looks like an O(n) operation.

Focus is on readability and not on performance, agreed.

chenkins · 2024-11-22T22:28:13Z

flatland/envs/agent_chains.py

-        svBlocked = self.find_stop_preds(svStops)
+        # Just look for the tree of preds for each voluntarily stopped agent (i.e. not wanting to move)
+        # TODO why not re-use mark_preds(swStops, color="red")?
+        # TODO refactoring suggestion: only one "blocked" red = 1. all deadlocked and their predecessor, 2. all predecessors of self-loopers, 3.


@hagrid67 @aiAdrian if the loop only contained conflict resolution for the remaining nodes of indegree > 1, that would greatly reduce reading complexity - do you see a problem with that? And also drop the red-purple distinction?

Yes I think the suggestion that we only need to process indegree>1 in a conflict resolution loop sounds correct.

I think the intention of the red / purple distinction is that

red is "temporary" ie blocked by a voluntarily stopped agent, or a slow one which is stepping in the same cell

purple is the result of a deadlock, which here means a "swap" where two agents are adjacent and facing, ie they are trying to "swap" cells. Purple is therefore "permanent" for the rest of the episode. I don't think we make any use of this but it could be useful.

Purple should take precedence over red, but I don't think it does. eg in the following chain:
1East -><- 2West <-3West <-4West
1 and 2 are deadlocked, so 3 and 4 should also be deadlocked: there is nothing they can do now to escape the deadlock.

But if 2 or 3 also has a failure, or maybe even chooses a voluntary "pause/stop" action, I think this in effect "breaks" the purple chain, in the implementation as I remember it. I remember partial chains flicking from purple to red and back. I think this is visually misleading but harmless (because they still don't move).

What happens is that if say agent 3 above has a malfunction, then it's "desired motion" becomes a self-loop which breaks the chain in our representation. (When I wrote this, I think all agent speeds were 1)

We could somehow preserve purple chains from one timestep to the next: once an agent is purple, it remains purple for the episode. We could then prevent those agents stepping, and reduce the processing in the motion check. It may also be useful in the observation (if it isn't already there).

I suspect that the very poor performance of motioncheck in the profiling test case may be related to random actions which result in huge jams and this provokes a lot of traversing of predecessor trees. If these were marked purple and preserved and then skipped, there could be performance benefit.

chenkins · 2024-11-22T22:28:49Z

flatland/envs/agent_chains.py

        # Get all the chains of agents - weakly connected components.
        # Weakly connected because it's a directed graph and you can traverse a chain of agents
        # in only one direction
+        # TODO why do we need weakly connected components at all? Just use reverse traversal of directed edges?


@hagrid67 @aiAdrian why do we need weakly connected components at all? We could just reverse traverse the directed graph?

Yes I think you are right once again! We could just traverse the reverse* graph, starting from the "swaps and stops" (ie immediate deadlocks, and voluntary / failed / slow agents). There doesn't seem to be any need to break the graph down into weakly connected components first. The traversal will naturally terminate as it reaches the "boundaries" of WCCs.

When I wrote the motioncheck I tried to look for algos which did clever stuff, assuming they would be faster than my own implementation. Processing the WCCs separately seemed reasonable and my small-case performance checks did not expose the O(n^2) performance problems we seem to have.

See the comment below - I think we still need the reverse graph because the dfs traversal to read the tree of blocked agents can only progress in a forward direction.
(*You said reverse-traverse the graph, I'm saying traverse the reverse graph ;)

chenkins · 2024-11-22T22:29:46Z

flatland/envs/agent_chains.py

@@ -12,31 +41,24 @@ class MotionCheck(object):
    """

    def __init__(self):
-        self.G = nx.DiGraph()
+        self.G = nx.DiGraph()  # nodes of type `Cell`
+        # TODO do we need the reversed graph at all?


@hagrid67 @aiAdrian why do we need the reversed graph at all? Can we not just use the predecessors of directed graphs? Or does this have performance drawbacks?

We need to traverse the desired motion graph backwards, to find agents which are trying to move into a blocked cell.

I don't think there is a NetworkX call to do this. dfs_predessors does the wrong thing: it traverses the tree in the forward direction, and simply returns the predecessor of each node.

For example, if cell 4 is blocked, we want all the (branched) predecessors 0,1,2,3,5,6. But dfs_predecessors returns an empty set because 4 has no successors.

G = nx.DiGraph([ (0,1), (1,2), (2,3), (3,4), (5,6), (6,2) ]) nx.dfs_predecessors(G, source=4) returns: {}

However we can get what we need using the reverse graph and dfs_postoder_nodes (as in the code):

H = G.reverse() list(nx.dfs_postorder_nodes(H, source=4)) returns: [0, 1, 5, 6, 2, 3, 4]

However this maybe raises the question, do we really need the forward graph? Maybe we should build only the reverse graph, because we only really need to follow the motion in reverse? We only seem to use G.succ in check_motion() and it doesn't seem to be doing much.

So if we built only the reversed graph Grev, we could use Grev.succ instead of G.pred.

Generally trying to navigate "upstream" in a Directed Graph seems like hard work in NetworkX.

@hagrid67 thought of:

g = nx.DiGraph([ (0,1), (1,2), (2,3), (3,4), (5,6), (6,2) ]) print(list(g.predecessors(4))) # returns: [3]

chenkins · 2024-11-22T22:30:29Z

flatland/envs/agent_chains.py

-        # Just look for the tree of preds for each voluntarily stopped agent
-        svBlocked = self.find_stop_preds(svStops)
+        # Just look for the tree of preds for each voluntarily stopped agent (i.e. not wanting to move)
+        # TODO why not re-use mark_preds(swStops, color="red")?


@hagrid67 @aiAdrian why do we need find_stop_preds - couldn't we not just re-use mark_preds(swStops, color="red")?

Gosh yes, well spotted, this does seem to be pointless; I'm embarrassed for my younger self!

FWIW, the logic in mark_preds (called block_preds in my older version) does not match the comment "only color those not already marked" but it looks like it overwrites the color. In my older version it seems to count the ones it has to change, which would justify the check; but then the count is discarded anyway.

aiAdrian · 2024-12-02T14:54:31Z

The documentation i can understand is looks good. but the detailed questions i can not answer - > please @hagrid67

hagrid67

I think the changes look good.
The questions also seem to suggest further improvements which I have commented on.

chenkins · 2024-12-04T12:47:02Z

flatland/envs/agent_chains.py

+    def find_swaps(self) -> Set[Cell]:
+        """
+        Find loops of size 2 in the graph, i.e. swaps leading to head-on collisions.
+        :return: set of all cells in swaps.


use numpydoc, see https://github.com/flatland-association/flatland-rl/blob/main/CONTRIBUTING.md#numpydoc

chenkins added 2 commits November 20, 2024 07:54

Extract test_agent_chains.py

9dfdf84

Update RailEnv documentation.

9051feb

chenkins commented Nov 20, 2024

View reviewed changes

tests/test_agent_chains.py Show resolved Hide resolved

chenkins added 4 commits November 20, 2024 11:00

Use find_conflicts as in core for agent chain tests as well.

2bc47c3

Add assertions.

b2b4a11

Fix agent close following notebook.

1db2e45

Remove unused code.

14b7f9b

chenkins requested review from hagrid67 and aiAdrian November 20, 2024 10:40

chenkins changed the title ~~86 improve documentation motion check~~ 86 Improve documentation MotionCheck Nov 20, 2024

chenkins commented Nov 21, 2024

View reviewed changes

tests/test_agent_chains.py Outdated Show resolved Hide resolved

chenkins commented Nov 21, 2024

View reviewed changes

flatland/envs/agent_chains.py Outdated Show resolved Hide resolved

chenkins added 3 commits November 22, 2024 20:43

Move agent chains render method from core to notebooks, its only usag…

1f09deb

…e location.

Add type hints, improve code comments, remove commented out code, add…

b50dac0

… refactoring suggestion.

Remove non-reachable code.

f54eee6

chenkins commented Nov 22, 2024

View reviewed changes

hagrid67 approved these changes Dec 4, 2024

View reviewed changes

chenkins commented Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

86 Improve documentation MotionCheck #87

86 Improve documentation MotionCheck #87

chenkins commented Nov 20, 2024

chenkins Nov 22, 2024

hagrid67 Dec 3, 2024

chenkins Dec 4, 2024

chenkins Nov 22, 2024

hagrid67 Dec 4, 2024 •

edited

Loading

chenkins Nov 22, 2024

hagrid67 Dec 4, 2024 •

edited

Loading

chenkins Nov 22, 2024

hagrid67 Dec 4, 2024 •

edited

Loading

chenkins Dec 4, 2024

chenkins Nov 22, 2024

hagrid67 Dec 4, 2024 •

edited

Loading

aiAdrian commented Dec 2, 2024

hagrid67 left a comment

chenkins Dec 4, 2024

86 Improve documentation MotionCheck #87

Are you sure you want to change the base?

86 Improve documentation MotionCheck #87

Conversation

chenkins commented Nov 20, 2024

Changes

Related issues

Checklist

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

hagrid67 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hagrid67 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hagrid67 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hagrid67 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

aiAdrian commented Dec 2, 2024

hagrid67 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hagrid67 Dec 4, 2024 •

edited

Loading

hagrid67 Dec 4, 2024 •

edited

Loading

hagrid67 Dec 4, 2024 •

edited

Loading

hagrid67 Dec 4, 2024 •

edited

Loading