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

7 piece syzygy TB support #557

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

Conversation

jjoshua2
Copy link
Contributor

@jjoshua2 jjoshua2 commented May 8, 2018

support 7 man syzygy and 16 bit dtz flag

support 7 man syzygy and 16 bit dtz flag
@jjoshua2
Copy link
Contributor Author

jjoshua2 commented May 8, 2018

for some reason this seems to be a regression? I think it has to be another patch in next...

Score of lczeroTB v8 id251 vs lczero7TB id251: 7 - 0 - 24 [0.613]
Elo difference: 79.83 +/- 54.31

@Tilps
Copy link
Contributor

Tilps commented May 8, 2018

Have you tested it with this patch but with the same tablebases on both sides? (To rule out tablebase performance issues?)

@jjoshua2
Copy link
Contributor Author

jjoshua2 commented May 8, 2018 via email

@jjoshua2
Copy link
Contributor Author

jjoshua2 commented May 10, 2018

last test had tilps nn cache difference. I think this would be safe to commit without regression...
1+1 sm 120

Score of lczero7TB id251 vs lczerov9 id251: 67 - 78 - 317 [0.488]
Elo difference: -8.27 +/- 17.73

it may be a small regression, but I think it is unlikely, since it started out positive at first. Any regression is probably due to 7 man being on HD instead of SSD like my 6 man, and the fact that I only have not useful ones really.

@@ -501,7 +501,7 @@ int decompress_pairs(PairsData* d, uint64_t idx) {
uint32_t k = idx / d->span;

// Then we read the corresponding SparseIndex[] entry
uint32_t block = number<uint32_t, LittleEndian>(&d->sparseIndex[k].block);
size_t block = number<uint32_t, LittleEndian>(&d->sparseIndex[k].block);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this change...

@@ -155,7 +155,7 @@ struct LR {
Sym get() {
return S == Left ? ((lr[1] & 0xF) << 8) | lr[0] :
S == Right ? (lr[2] << 4) | (lr[1] >> 4) :
S == Value ? lr[0] : (assert(false), Sym(-1));
S == Value ? ((lr[1] & 0xF) << 8) | lr[0] : (assert(false), Sym(-1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the comment above to indicate that value can also be 12 bits now?

if (flags & TBFlag::Mapped)
value = map[idx[WDLMap[wdl + 2]] + value];
if (flags & TBFlag::Mapped) {
if (flags & TBFlag::LongDTZ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is a bit messed up here and the following few lines.

for (int i = 0; i < 4; ++i) { // Sequence like 3,x,x,x,1,x,0,2,x,x
e.get(0, f)->map_idx[i] = (uint16_t)(data - e.map + 1);
data += *data + 1;
int flags = e.get(0, f)->flags;
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation wrong on this line.

@ASilver
Copy link

ASilver commented May 18, 2018

While I think it is cool, I wonder how many people will have 20TB of tablebases... I should also add that many many GUIs take care of this themselves, with no need for code from the engine. I presume Leela is not using the TBs within its search.

@jjoshua2
Copy link
Contributor Author

jjoshua2 commented May 18, 2018

No GUI does 7 piece syzygy currently, and some common ones don't support syzygy at all. Actually, cutechess is the only one I know that does.

And there is no reason to comment on a PR if you don't bother to understand it enough to know what it does. Leela does use TB within search...

And if you are only interested in analyzing one position, you only need that one 7 piece syzygy file, or a few that an 8 piece position might convert to, so you could use it with only a few GB of files. Even all the pawnless files all fit on my 4TB hard drive with room to store other things too. But there is no reason to limit an engine because few people would use it, see TCEC.

@ASilver
Copy link

ASilver commented May 18, 2018 via email

@jjoshua2
Copy link
Contributor Author

I haven't used a Chessbase GUI for a few years, but I've used Arena and InfinityChess and Cutechess recently. Mostly cutechess. But this whole GUI thing is irrelevant, because the engine needs them for search and for root filtering.

@ASilver
Copy link

ASilver commented May 18, 2018

Then you could simply have said it uses the syzygy within its search, instead of telling me (pretty rudely), editor of ChessBase News for 8 years, CB and Fritz cannot use them. Especially in view of the fact that CB sells more than one syzygy package in their store.

@jjoshua2
Copy link
Contributor Author

I apologize for the error about CB/Fritz. I didn't know you were associated with CB either, so easier to take an offense on your product. I mean no disrepect to it, although we are not going to optimize Leela for one particular GUI.

I did clearly state that it is used in search, and if you had looked at the code you would have seen that before I even told you.

This area is for code comments. If you are interested in more general things, please use the issues section.

@jjoshua2
Copy link
Contributor Author

jjoshua2 commented May 18, 2018

This whole PR might be irrelavent anyway since it's been a while without being merged, and focus is supposed to be on lc0 cudann now, which won't easily support syzygy. But for those that want it, they can use this build in the meantime since it is published now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants