Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial dfrc support #1684

Merged
merged 4 commits into from
Dec 4, 2022
Merged

initial dfrc support #1684

merged 4 commits into from
Dec 4, 2022

Conversation

borg323
Copy link
Member

@borg323 borg323 commented Jan 4, 2022

Very lightly tested

@mooskagh mooskagh self-requested a review June 30, 2022 07:16
src/chess/board.h Outdated Show resolved Hide resolved
}

private:
// Position of "left" (queenside) rook in starting game position.
std::uint8_t queenside_rook_ : 3;
std::uint8_t our_queenside_rook_ : 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, do we even care about board representation size anymore? I think we don't, we actually care about performanse more.
It probably makes sense to remove all bit fields and have just a byte per field, and split data_ into 4 bools (not sure about this part).

Also, we mix std::uint8_t and uint8_t a lot in this file, probably makes sense to remove std::.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left data_ as an int8_t for the time being.

src/chess/board.h Outdated Show resolved Hide resolved
src/chess/board.h Outdated Show resolved Hide resolved

// Note: this is not a strict xfen compatible output. Without access to the
// board its not possible to know whether there is ambiguity so all cases
// with any non-standard rook positions are encoded in the x-fen format
std::string as_string() const {
if (data_ == 0) return "-";
std::string result;
if (queenside_rook() == FILE_A && kingside_rook() == FILE_H) {
if (our_queenside_rook() == FILE_A && our_kingside_rook() == FILE_H &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tempting to create a helper struct, something like

struct RookPair {
   uint8 kingside;
   uint8 queenside;
   bool can_castle_queenside;
   bool can_castle_kingside;

   bool is_standard_chess() const { return queenside == FILE_A && kindside == FILE_H; }
   // and other similar functions.
};

Not sure if worth it though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started doing it, but didn't see a lot of potential, so I gave up.

src/chess/board.cc Outdated Show resolved Hide resolved
src/chess/board.cc Show resolved Hide resolved
// Finding rightmost rook.
for (right_rook = FILE_H; right_rook > king_col; --right_rook) {
if (rooks.get(is_black ? RANK_8 : RANK_1, right_rook)) break;
for (our_right_rook = FILE_H; our_right_rook > king_col;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels that this code should be refactored, too duplicated..

Maybe have a function to find lowest and highest bit on the bit level (on "our rooks" bitmask)?

But I guess it let's decide whether to factor our struct RookPair first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some refactoring for now.

src/neural/decoder.cc Outdated Show resolved Hide resolved
src/neural/encoder.cc Outdated Show resolved Hide resolved
@Naphthalin Naphthalin added the testing required Feature/bug fix needs more testing. Implies not for merge. label Nov 2, 2022
Copy link
Member

@mooskagh mooskagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may worth checking the performance with trivial/random backend, but I'm certain there's no performance impact.

@borg323
Copy link
Member Author

borg323 commented Dec 4, 2022

Actually there is a ~1% speed gain on a single thread trivial backend test.

@borg323 borg323 merged commit 23768b7 into LeelaChessZero:master Dec 4, 2022
@borg323 borg323 deleted the dfrc branch December 4, 2022 11:32
borg323 added a commit to borg323/lc0 that referenced this pull request Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing required Feature/bug fix needs more testing. Implies not for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants