search

LeelaChessZero / lc0

The rewritten engine, originally for tensorflow. Now all other backends have been ported here.
GNU General Public License v3.0
2.4k stars 525 forks source link

Incorrect comment on bit order in class BitBoard? #825

Open rhalbersma opened 5 years ago

rhalbersma commented 5 years ago

In bitboard.h, for class BoardSquare the comment on line 43/44 states:

// As a single number, 0 to 63, bottom to top, left to right.
// 0 is a1, 8 is a2, 63 is h8.

In contrast, for the class BitBoard the comment on line 88/89 states:

// Bit enumeration goes from bottom to top, from left to right:
// Square a1 is bit 0, square a8 is bit 7, square b1 is bit 8.

However, the subsequent BitBoard member functions set, reset etc. take a BoardSquare parameter. So in fact BitBoard has the same ordering as BoardSquare.

Should this comment in class BitBoard be updated?

mooskagh commented 5 years ago

Yeah initially it was wrong in both places, but then fixed in one but apparently not another.

rhalbersma commented 5 years ago

There are some other improvements possible in bitboard.h. E.g. BitBoard::Mirror() could use the compiler intrinsics _byteswap_uint64 (MSVC) or __builtin_bswap64 (gcc/clang). This should reduce the current 15 instructions to 1 on modern architectures.

And ReverseBitsInBytes from mcts/node.cc could be moved to be a member function of BitBoard, and renamed MirrorVertical e.g. (and Mirror be renamed to MirrorHorizontal).

I also don’t quite understand the rationale to vertically mirror the inputs for training, could you explain?

I could send a PR later. What do you think?

mooskagh commented 5 years ago

Those are good suggestsions, PR will be appreciated!

TesseractA commented 4 years ago

Still Relevant?

mooskagh commented 4 years ago

Yes, I've just check, still wrong (the comments issue in the very first comment). :)