Leela-zero: Add unit tests for FullBoard

[barrybecker4]

Now that I have experience from adding gtests for FastBoard, I would like to add them for FullBoard.
I discovered, while adding tests for FastBoard, that some of the methods (count_pliberties, is_eye, is_suicide) only make sense to test in the context of a FulBoard.

If it makes sense to do so, I can open issues for the following and implement them as time permits:

• Either remove set/get_square (because it is unused) or make m_square private in FastBoard and allow access only with set/get_square. My preference is for the latter.
• Refactor out FastBoardSerializer class from FastBoard. I think it would simplify the code to extract all the logic having to do with board serialization to a separate class. It would have methods for serialize, text_to_move, text_to_move_sgf and probably starpoint.
• I think there should be an update_board method in FastBoard that contains all the logic from FullBoard::update_board except the part that deals with the Zobrist hashing. Then the FullBoard::update_board method would just do the Zobrist update and call FastBoard::update_board. If we did this, then we could create sample FastBoards in the unit tests and tests for count_pliberties, is_eye, is_suicide, and area_score would make sense. As it is now, it really only makes sense to test those methods in the context of a FullBoard.

[barrybecker4]

While writing the FullBoard tests to validate is_eye(v), I think I may have discovered a bug. Hopefully it is just my misunderstanding.
All the cases make sense to me except for the last one in the list below (the point at "b5").

/*
  Only single points surrounded by own color are counted as eyes.
      a b c d e
    5 X . X X .  5
    4 . . X . X  4
    3 X X . X .  3
    2 . O X O .  2
    1 . . . . .  1
      a b c d e
*/
TEST(FullBoardTest, IsEyeOnBlackField) {
    FullBoard b = create_5x5_all_black();
    EXPECT_EQ(true, b.is_eye(FastBoard::BLACK, b.get_vertex(4, 4))); // black eye
    EXPECT_EQ(true, b.is_eye(FastBoard::BLACK, b.get_vertex(3, 3))); // black eye
    EXPECT_EQ(false, b.is_eye(FastBoard::WHITE, b.get_vertex(3, 3))); // not white eye
    EXPECT_EQ(false, b.is_eye(FastBoard::BLACK, b.get_vertex(2, 2))); // potentially false eye, so not real eye
    EXPECT_EQ(false, b.is_eye(FastBoard::BLACK, b.get_vertex(1, 1))); 
    EXPECT_EQ(false, b.is_eye(FastBoard::BLACK, b.get_vertex(0, 3))); // a4 is similar to b5, but not an eye
    EXPECT_EQ(false, b.is_eye(FastBoard::BLACK, b.get_vertex(1, 3))); // not single point eye
    EXPECT_EQ(false, b.is_eye(FastBoard::BLACK, b.get_vertex(4, 2)));
    EXPECT_EQ(false, b.is_eye(FastBoard::BLACK, b.get_vertex(3, 0)));
    EXPECT_EQ(false, b.is_eye(FastBoard::BLACK, b.get_vertex(1, 1)));
    EXPECT_EQ(false, b.is_eye(FastBoard::WHITE, b.get_vertex(1, 1)));
    EXPECT_EQ(false, b.is_eye(FastBoard::WHITE, b.get_vertex(2, 0)));
    EXPECT_EQ(false, b.is_eye(FastBoard::WHITE, b.get_vertex(4, 2)));
    EXPECT_EQ(true, b.is_eye(FastBoard::BLACK, b.get_vertex(1, 4))); // b5 should not be eye? bug?
}

To try and debug I printed all the relevant values at the start of is_eye(get_vertex(1, 4)).
ownsurrounded=4 i=(1, 4) m_neighbours=20 m_eyemask[0]=4
ownsurrounded = 20 (10100) & 4 (100) = 4 (100)

I expected ownsurrounded to be 0/false in this case because it is surrounded on only 3 sides, not 4.
I don't fully understand how m_neighbours of 20 was arrived at because I don't understand the bit twiddling that is done in add_neighbor when update_board is called. My guess is that the 20 value for m-neighbours is wrong, but I'm not sure why.

The symmetrical case of "a4" is (correctly) not considered an eye. If it's not an eye, I don't think "b5" should be either.
The values at "a4" are:
ownsurrounded=0 i=(0, 3) nbrs=275 eyemask[0]=4

For reference, this is how I create the FullBoard under test above. As you can see, all the black stones are placed first, then 2 white stones. There is nothing unusual happening - like captures or kos.
FullBoard create_5x5_all_black() { FullBoard b; b.reset_board(5); b.update_board(FastBoard::BLACK, b.get_vertex(1, 2)); b.update_board(FastBoard::BLACK, b.get_vertex(2, 1)); b.update_board(FastBoard::BLACK, b.get_vertex(0, 4)); b.update_board(FastBoard::BLACK, b.get_vertex(2, 3)); b.update_board(FastBoard::BLACK, b.get_vertex(2, 4)); b.update_board(FastBoard::BLACK, b.get_vertex(2, 4)); b.update_board(FastBoard::BLACK, b.get_vertex(3, 2)); b.update_board(FastBoard::BLACK, b.get_vertex(3, 4)); b.update_board(FastBoard::BLACK, b.get_vertex(4, 3)); b.update_board(FastBoard::BLACK, b.get_vertex(0, 2)); b.update_board(FastBoard::WHITE, b.get_vertex(1, 1)); b.update_board(FastBoard::WHITE, b.get_vertex(3, 1)); return b; }

[Hersmunch]

I don't know if it matters but create_5x5_all_black() places two black stones at c5.

[barrybecker4]

Thank you @Hersmunch. That was indeed the problem. Once I removed that duplicate placement at (2, 4) then it no longer reports that it's an eye. There is actually an assertion that checks for duplicate placements, but I have not worked out how to turn on assertions in the unit tests yet, so it did not trigger.

[wnwen]

I think asserts are turned on for debug builds? With cmake -DCMAKE_BUILD_TYPE=Debug .. in build.