Make compact minimax an option for simple agent#334
Conversation
| let position_bits = bits_needed(num_player_positions - 1); | ||
| let walls_remaining_bits = bits_needed(max_walls); | ||
| let steps_bits = bits_needed(max_steps); | ||
| let completed_steps_bits = bits_needed(max_steps); |
There was a problem hiding this comment.
These renamings are just to make things more explicit
| @@ -329,53 +329,6 @@ impl QGameMechanics { | |||
| true | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
is_move_valid isn't fully implemented, and isn't actually used yet. To reduce confusion I've removed it for now.
| let moves = mechanics.get_valid_moves(data); | ||
| let mut actions: Vec<(usize, usize, usize)> = | ||
| moves.into_iter().map(|(row, col)| (row, col, 0)).collect(); | ||
| moves.into_iter().map(|(row, col)| (row, col, 2)).collect(); |
There was a problem hiding this comment.
This was a nasty bug - at some point claude decided to use 0 for moves and 1, 2 for vertical/horizontal walls in the compact representation. Everywhere else we use 0 and 1 for vertical/horizontal walls and 2 for movements. This led to minimax returning actions which looked invalid to our python code.
| let opponent = 1 - current_player; | ||
|
|
||
| if completed_steps >= mechanics.repr().max_steps() { | ||
| if mechanics.repr().get_completed_steps(data) >= mechanics.repr().max_steps() { |
There was a problem hiding this comment.
Completed_steps is tracked in the state vector, so we don't need a separate variable for it.
| completed_steps: usize, | ||
| max_depth: usize, | ||
| search_depth: usize, | ||
| max_search_depth: usize, |
There was a problem hiding this comment.
Search depth and completed steps are the same when starting with a new game, but otherwise are different. For clarity we now track them separately.
|
|
||
| // Apply action | ||
| if *action_type == 0 { | ||
| if *action_type == 2 { |
There was a problem hiding this comment.
When i switched to using 2 to represent moves, I completely missed that this section of code also needed to be changed. Led to some more annoying errors. Makes me think we should really use rust types to make this all clearer, and avoid doing hacky things like orientation = *action_type - 1; below. I'll change that later though.
There was a problem hiding this comment.
An even better thing for vibe coding would probably be to have unit tests with large coverage (AI generated) to catch these things.
| use compact::q_game_mechanics::QGameMechanics; | ||
|
|
||
| // Create QBitRepr and QGameMechanics | ||
| let repr = QBitRepr::new(board_size, max_walls, max_steps as usize); |
There was a problem hiding this comment.
QGameMechanics creates its own QBitRepr which we can use, so we didn't need this separate one.
| action = MoveAction((action_array[0], action_array[1])) | ||
| elif action_array[2] == qgrid.ACTION_WALL_VERTICAL: | ||
| action = WallAction((action_array[0], action_array[1]), qgrid.WALL_ORIENTATION_VERTICAL) | ||
| action = WallAction((action_array[0], action_array[1]), WallOrientation.VERTICAL) |
There was a problem hiding this comment.
This is an unrelated fix - qgrid.WALL_ORIENTATION_* are just integers - we usually want to use WallOrientation.* which have the same integer value internally but also have a nice name that can be used to print them out.
|
LGTM |
2 participants
By running against simple with the other minimax implementations, I was able to find a few bugs which I also fix in this PR. There's still some bug in the compact minimax when using walls; I'll leave that to another PR.