Make Maze Generation Iterative

[jbdyn]

Our binary space partitioning routine has been implemented recursively.
Now its iterative.

I ran pyinstrument on this benchmark script

# benchmark.py
from pelita.maze_generator import generate_maze

for _ in range(1000):
    generate_maze()

and saw a slight performance improvement from 0.247 s to 0.232 s in add_wall_and_split.
I used pyinstrument --show=generate_half_maze benchmark.py to filter the output a bit.

before

  _     ._   __/__   _ _  _  _ _/_   Recorded: 22:00:04  Samples:  2614
 /_//_/// /_\ / //_// / //_'/ //     Duration: 2.616     CPU time: 2.610
/   _/                      v5.1.1

Program: benchmark.py

2.615 <module>  benchmark.py:1
├─ 2.379 generate_maze  pelita/maze_generator.py:266
│  ├─ 1.117 find_trapped_tiles  pelita/maze_generator.py:58
│  │  ├─ 0.969 biconnected_components  networkx/algorithms/components/biconnected.py:170
│  │  │     [11 frames hidden]  networkx, <built-in>
│  │  │        0.908 _biconnected_dfs  networkx/algorithms/components/biconnected.py:338
│  │  └─ 0.098 [self]  pelita/maze_generator.py
│  ├─ 0.565 walls_to_graph  pelita/team.py:43
│  │  ├─ 0.443 Graph.add_edge  networkx/classes/graph.py:906
│  │  │     [4 frames hidden]  networkx, <built-in>
│  │  └─ 0.116 [self]  pelita/team.py
│  ├─ 0.276 generate_half_maze  pelita/maze_generator.py:213
│  │  └─ 0.247 add_wall_and_split  pelita/maze_generator.py:121
│  │     └─ 0.223 add_wall_and_split  pelita/maze_generator.py:121
│  │        └─ 0.196 add_wall_and_split  pelita/maze_generator.py:121
│  │           └─ 0.146 add_wall_and_split  pelita/maze_generator.py:121
│  │              └─ 0.110 add_wall_and_split  pelita/maze_generator.py:121
│  │                 └─ 0.064 add_wall_and_split  pelita/maze_generator.py:121
│  │                    └─ 0.031 add_wall_and_split  pelita/maze_generator.py:121
│  ├─ 0.188 argmap_is_connected_1  <class 'networkx.utils.decorators.argmap'> compilation 5:1
│  │     [3 frames hidden]  networkx
│  ├─ 0.071 distribute_food  pelita/maze_generator.py:87
│  │  └─ 0.067 sample_nodes  pelita/maze_generator.py:49
│  │     ├─ 0.033 Random.sample  random.py:363
│  │     └─ 0.027 sorted  <built-in>
│  ├─ 0.053 sorted  <built-in>
│  ├─ 0.050 mirror  pelita/maze_generator.py:43
│  └─ 0.047 [self]  pelita/maze_generator.py
├─ 0.201 <module>  pelita/__init__.py:1
│  └─ 0.200 <module>  pelita/game.py:1
│     ├─ 0.127 <module>  pelita/team.py:1
│     │  └─ 0.126 <module>  networkx/__init__.py:1
│     │     └─ 0.068 <module>  networkx/algorithms/__init__.py:1
│     └─ 0.047 <module>  pelita/viewer.py:1
│        └─ 0.038 <module>  rich/console.py:1
└─ 0.035 [self]  benchmark.py

after

  _     ._   __/__   _ _  _  _ _/_   Recorded: 21:45:04  Samples:  2637
 /_//_/// /_\ / //_// / //_'/ //     Duration: 2.641     CPU time: 2.634
/   _/                      v5.1.1

Program: benchmark.py

2.640 <module>  benchmark.py:1
├─ 2.390 generate_maze  pelita/maze_generator.py:280
│  ├─ 1.137 find_trapped_tiles  pelita/maze_generator.py:58
│  │  ├─ 0.986 biconnected_components  networkx/algorithms/components/biconnected.py:170
│  │  │     [13 frames hidden]  networkx, <built-in>
│  │  └─ 0.093 [self]  pelita/maze_generator.py
│  ├─ 0.561 walls_to_graph  pelita/team.py:43
│  │  ├─ 0.444 Graph.add_edge  networkx/classes/graph.py:906
│  │  │     [5 frames hidden]  networkx, <built-in>
│  │  └─ 0.114 [self]  pelita/team.py
│  ├─ 0.254 generate_half_maze  pelita/maze_generator.py:227
│  │  └─ 0.232 add_wall_and_split  pelita/maze_generator.py:121
│  │     ├─ 0.079 Random.randint  random.py:336
│  │     │     [2 frames hidden]  random
│  │     ├─ 0.068 [self]  pelita/maze_generator.py
│  │     └─ 0.061 Random.shuffle  random.py:354
│  │        └─ 0.037 Random._randbelow_with_getrandbits  random.py:245
│  ├─ 0.200 argmap_is_connected_1  <class 'networkx.utils.decorators.argmap'> compilation 5:1
│  │     [6 frames hidden]  networkx, <built-in>
│  ├─ 0.066 distribute_food  pelita/maze_generator.py:87
│  │  └─ 0.054 sample_nodes  pelita/maze_generator.py:49
│  │     └─ 0.031 Random.sample  random.py:363
│  ├─ 0.060 sorted  <built-in>
│  ├─ 0.049 mirror  pelita/maze_generator.py:43
│  └─ 0.042 [self]  pelita/maze_generator.py
├─ 0.207 <module>  pelita/__init__.py:1
│  └─ 0.206 <module>  pelita/game.py:1
│     ├─ 0.131 <module>  pelita/team.py:1
│     │  └─ 0.131 <module>  networkx/__init__.py:1
│     │     └─ 0.071 <module>  networkx/algorithms/__init__.py:1
│     └─ 0.048 <module>  pelita/viewer.py:1
│        └─ 0.039 <module>  rich/console.py:1
└─ 0.043 [self]  benchmark.py

The tests, especially the maze stability test, pass.

The logic for creating the walls did not change, but maybe this would be something for another PR anyway?

[jbdyn]

Force-pushed to remove changes introduced by running ruff locally.

[otizonaizit]

Oh wow, this is very cool, thanks!

Given that you don't seem to have changed the order of things, this new implementation should give the very same mazes as the old recursive one if started with the same random seed, right? Can you test that?

[otizonaizit]

Oh wow, this is very cool, thanks!

Given that you don't seem to have changed the order of things, this new implementation should give the very same mazes as the old recursive one if started with the same random seed, right? Can you test that?

I mean, I trust test_generate_maze_stability a lot, but still I think for future reference it would be nice to have shown at least once that we can generate, say, 1000 mazes and they are all identical with the two implementations...

Otherwise I am very very happy to see recursion gone. Now I understand better what happens. And it makes it also clear why you need an infinite loop and why you can not know in advance the number of partitions.

The infinite loop is guaranteed to exit? I am not sure I can prove that...

[jbdyn]

Given that you don't seem to have changed the order of things, this new implementation should give the very same mazes as the old recursive one if started with the same random seed, right? Can you test that?

I tested with

# on main
for i in $(seq 100); do pelita --seed $i --store-layout old/$i.layout; done

# on make-maze-generation-iterative
for i in $(seq 100); do pelita --seed $i --store-layout new/$i.layout; done

and

diff -r -q old new

which shows nothing (every file's content is the same in both directories) and

sha256sum old/* | awk '{print $1 }' | old.txt
sha256sum new/* | awk '{print $1 }' | new.txt

which yields old.txt and new.txt to be identical (same checksum).

1000 mazes would have taken a fair amount of time, but I think 100 mazes is also quite assuring.

[jbdyn]

The infinite loop is guaranteed to exit? I am not sure I can prove that...

It is always exiting, since the position of the walls pos is in [xmin + 1, xmax - (xmin + 1)] or [ymin + 1, ymax - (ymin + 1)], respectively, and thus always yielding partitions smaller than the current partition.
The checks for height < 3, width < 3 and partition_length < rng.randint(3, 5) ensure no further addition of partitions, and p += 1 in those checks and after partitioning ensures that we always advance in the list of partitions.

So, partitions always shrink, no new partitions are added once they shrank below a threshold, and the loop increases the list index in every case.

[otizonaizit]

I tested with
[...]

1000 mazes would have taken a fair amount of time, but I think 100 mazes is also quite assuring.

Yes, thanks, perfect!

[otizonaizit]

The infinite loop is guaranteed to exit? I am not sure I can prove that...

It is always exiting, since the position of the walls pos is in [xmin + 1, xmax - (xmin + 1)] or [ymin + 1, ymax - (ymin + 1)], respectively, and thus always yielding partitions smaller than the current partition.
The checks for height < 3, width < 3 and partition_length < rng.randint(3, 5) ensure no further addition of partitions, and p += 1 in those checks and after partitioning ensures that we always advance in the list of partitions.

So, partitions always shrink, no new partitions are added once they shrank below a threshold, and the loop increases the list index in every case.

Great explanation, could you add this as a comment in the function for future reference?

[otizonaizit]

Thanks , that's very straightforward. If you could add the explanation of the guaranteed loop exit in the function I will merge right away ;)

[jbdyn]

Done ☺️

[Debilski]

👍

Not sure if the partition index p and keeping the used partitions is needed for clarity, but I think you can rewrite it a little simpler with partition = partitions.pop() and partitions.append(new[1]) ... (or extend but then the order in new needs to be reversed) and dropping the partition index altogether.

[jbdyn]

Yes, you are right, we could drop that index and realize the routine with

partitions.pop(0)
...
partitions.insert(0, new[1])
partitions.insert(0, new[0])

The index 0 is important here for ensuring maze stability, as partitions are split depth-first (for now).

Also, I am thinking about a collections.deque instead of a list.

This would be a point for the finalization PR for the maze generation, alongside getting all the TODOs done in the maze_generator module.

[Debilski]

But if you pop and append from the end of the list, it should work just the same, no? (And have a better performance theoretically.)

[jbdyn]

Yes, it should.

[otizonaizit]

But if you pop and append from the end of the list, it should work just the same, no? (And have a better performance theoretically.)

Or we make it a deque ?

[jbdyn]

We can try all of the suggestions. Not to much work. In the end, well written comments are the most important thing to keep the algorithm understandable.

[jbdyn]

See #946 for changes.