-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Threefold repetition detection #925
Conversation
A nice effort with a satisfactory result. Congrats. |
Why -3, 1? It adds code, so it should be 0, 5 |
Can be considered a bug fix, therefore tested for no-regression. |
@Stefano80 Similar tries were also tested with bugfix bounds in the past. But I admit this patch adds a LOT of code. OTOH, this is the first one that passed STC and LTC! |
Good work! The 3-fold repetition blindness has been always annoying. |
Because the root position itself is reenterable unless it is a repetition (like in the case of bench). Why? I tested the alternative and it was weaker. |
It does not seem to make sense to allow a first repetition of the root position. Did keeping the old behaviour for the root position fail on fishtest? A possible way to remove the st->draw flags: after setting up the position in uci.cpp, walk from the root back into history and set all non-repeated st->key values to zero. |
Yes, it did.
Interesting idea. I'll try it. |
@saproj The fact that the first version failed SPRT(-3,1) does not mean it is weaker. You cannot draw any conclusion from a failed SPRT(-3,1). Also in case of a small regression (like -1 elo as could be the case here) you can make a SPRT(-3,1) pass by repeating it enough times (possibly combining it with placebo changes a la "Take 1", "Take 2", "optimized version" etc...). I think in this case the maintainers should simply be ready to (possibly) sacrifice 1 elo in order to repair what is obviously a bug in Stockfish. |
I know I don't have a vote - but I would vote for this patch. There are many users of Stockfish that would appreciate the nuance corrected here. |
I really don't understand why it should be tested with [-3, 1]. It it adds more than 30 lines of code. It does not fix anything, and for certain not a bug. It has not been approved in advance by maintainers. It is just an arbitrary abuse of the framework. The guidelines say:
Please either stick to the guidelines or propose to change them. @vdbergh : it has been repeated hundreds of times that conter-intuitive eval in an arbitrary class of positions is not considered a bug. |
I think 30 lines is an exaggeration. Some is documentation, spacing, and brackets. I see 13 without that. It fixes a problem and it does not appear to hurt. And is it possible that this might help when contempt is present achieving more at a lower setting? That is just a guess, not an assertion. At the very least, it means less grumbles on the forum. That has to be worth something. |
@vdbergh I did direct comparison locally. Not allowing to reenter the root position is, surprisingly, worse than allowing it. By several ELO points. Why? It's a mystery. |
@Stefano80 Just to clarify my position: I believe I'm fixing a bug. Surprised by your "for certain not a bug". |
Regardless we can agree, "It has not been approved in advance by maintainers."
How is "cp 0" a bug? (In my fork I do address this issue, however I cannot prove "cp 0" is a bug according to terminology used by maintainers.) |
To understand why this is a "bug" is fairly straightforward - the current behavior simply does not adhere to the actual rules of chess. Using a small eval difference is a bit disingenuous. Say the eval is -10, but human players repeated the previous position. Showing an eval of draw is clearly in error, since the first repetition does not result in an immediate draw under the actual rules of the game. While it may not make any difference for Stockfish in playing a game, it does cause a clear discontinuity in evaluation when used for analysis. As analysis is the primary use case for the engine, ensuring correct output should at least be worthy of consideration, IMHO. |
@syzygy1 Zeroing non-repeated st->key (at least the way I implemented it) is not better. See http://tests.stockfishchess.org/tests/view/58514cd80ebc5903140c5c04 |
@ddugovic, You mentioned you address this issue in your fork. I do not know about it. So, is there also a fix for the 1st problem from my list? I mean the problem when SF makes a loosing move in a drawn position. |
My fork's change probably looks familiar to some; unfortunately I forget who the original author is that I copied the idea from. People keep using the words "bug" and "fix" and while I only endorse the word "problem" my fork does not play |
@ddugovic Familiar idea. It introduces an ELO-stealing slowdown because of additional loop iterations. |
Congrats to the author !!! There is one helper function, and a few line changes. Let me take some examples: If search would never consider promotions to bishop, (hello old Rybka), If I have a car which runs perfectly, but would stop working each time I "leave from work" (but would be ok if I leave from all the other places), I would say that this is a problem. As it is, each time current master "leaves from a repetition", it virtually stops doing its duty which is It's not like a specific funny position which is misevaluated. This is a systemic error which does not really depend of the position. It depends only to the fact that there was a repetition in the game history. We expect an engine to properly handle e.p., castling rights, 50 moves rule, (which all relates to the game history) AND also repetitions. It might even be a small ELO gainer. In a time limited game, SF might go for a first repetition, and on the next move. using the info in the TT, and with the time allocated by the time management, it might go for a better fighting continuation or simply keep trying with something else. |
Having tried myself to fix this problem (bug?) and failing badly, I know how complicated it is if you dig deeper. The author of this patch did a good job, improving on his previous idea. |
@saproj |
@syzygy1, but your version treats the root position differently: it doesn't allow repeating it even once during a search, unlike saproj's version. He has already written about this here. |
@syzygy1 This idea of zeroing st->key looked appealing to me at first. But now that I have tested it, I want to stick to adding st->draw. By the way, this new field resides in an alignment gap (under x86_64) and StateInfo's size did not change. |
I'm in favor of this patch, even if it adds some complexity. To remove all doubt, we might consider a reverse simplification test (master against patch instead of patch versus master). |
If I were a maintainer i'd list a few "exceptions":
For which i'd relax the rules slightly. |
I guess this patch cuts to the root of Stockfish's purpose. |
@mcostalba Nice! But this again excludes the root position to be repeated once without directly assigning a draw score ... |
@mcostalba If I read correctly this is precisely the version that failed several times in the past. It seemed to be a -1 elo regression. So if you keep on testing it, it will eventually pass... which would be a good thing IMHO. |
@vdbergh Can you please post some link to validate your statement or you just go by
memory.
FYI this is fully equivalent to the original patch but the recursion at
root.
…On Sunday, December 25, 2016, vdbergh ***@***.***> wrote:
@mcostalba <https://github.com/mcostalba> If I read correctly this is
precisely the version that failed several times in the past. It seemed to
be a -1 elo regression. So if you keep on testing it, it will eventually
pass... which would be a good thing IMHO.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#925 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABDGAQxZSPE0dumwJ5gVi9qNLe097ekEks5rLscngaJpZM4LMEQh>
.
|
@mcostalba AFAICS your version does not contain the optimization "calc_draw". This is probably a placebo but one cannot be sure. |
This test shows that it's better to allow repetition of the root position: http://tests.stockfishchess.org/tests/view/58514cd80ebc5903140c5c04 |
@mcostalba This seems to be the first version. It failed SPRT(-4,0). |
These version are similar but not equivalent, in particular these have not
verify that the boundary is exacty at root position (it is tricky to find
the exact boundary, I did this by some debug code. You can see it in
previous commit of my pushed branch).
…On Sun, Dec 25, 2016 at 10:48 PM, vdbergh ***@***.***> wrote:
@mcostalba <https://github.com/mcostalba> This seems to be the first
version. It failed SPRT(-4,0).
https://groups.google.com/forum/?fromgroups=#!searchin/
fishcooking_results/3fold_fix%7Csort:relevance/fishcooking_
results/Jvg9Ytp5mY4/JyQ7uviPMkIJ
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#925 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABDGAby-yLS_WR1cHnEOKCB4m7Z9x6kzks5rLuTIgaJpZM4LMEQh>
.
|
@vdbergh regarding the calc_draw(), well if the patch passed just because of that then it is better to fail and do not commit, it means it passed by pure luck. In real games the cases where we reach plies before root are very very few (measured with debug instrumentation) and even in those cases, the extra search to reach the game ply base is absolutely unmeasurable. Again, if this is the case then it is much more probable to explain the successful test by a statistical fluke. |
@mcostalba Thanks for repeating my points (your last post). As for your earlier post. I think the old version from 2014 is equivalent to your "new" one (there is nothing tricky about this). I have not checked it carefully however (it was not my patch). |
Based on official-stockfish/Stockfish#925 bench: 5255881
Based on official-stockfish/Stockfish#925 bench: 5493489
BTW I was simply responding to your claim: "FYI this is fully equivalent to the original patch but the recursion at root.". This statement is false as the (probably placebo) optimization is dropped. Also: please do not use "FYI". I am perfectly capable of reading your code. |
Ok, I have submitted a couple of test: with root position included and excluded from extended draw repetition. |
I apologize for the untimeliness of the following idea as I do not fully understand HGM's comment as it applies to Stockfish:
Given that the |
@ddugovic, I've actually tried this idea on Fishtest: http://tests.stockfishchess.org/tests/view/58223a4b0ebc5910626b9c64 |
Both tests passed, so I think I will commit the one with root position excluded from extended draw repetition because it seems the most logical to me. |
@mcostalba, but only the version with the root position included works correctly with MultiPV > 1, as I have said earlier. |
We can also make a match between the two versions to decide which is stronger. |
Based on official-stockfish/Stockfish#925 bench: 5255881
Based on official-stockfish/Stockfish#925 bench: 5493489
@atumanian The argument on MultiPV is not clear, in particular has been tentatively refuted by @syzygy1 and I think his arguments make sense. Overall to include root because of MultiPV leads to a questionable and subjective argumentation for both parts, not to a clear black or white condition. so because it is not clear the benefit in MultiPV case and because, apart form this very specific and debatable point, there is no logical/measurable added benefit, I will commit the version with root excluded. |
Based on official-stockfish/Stockfish#925 bench: 5255881
Based on official-stockfish/Stockfish#925 bench: 5493489
Implement a threefold repetition detection. Below are the examples of problems fixed by this change. Loosing move in a drawn position. position fen 8/k7/3p4/p2P1p2/P2P1P2/8/8/K7 w - - 0 1 moves a1a2 a7a8 a2a1 The old code suggested a loosing move "bestmove a8a7", the new code suggests "bestmove a8b7" leading to a draw. Incorrect evaluation (happened in a real game in TCEC Season 9). position fen 4rbkr/1q3pp1/b3pn2/7p/1pN5/1P1BBP1P/P1R2QP1/3R2K1 w - - 5 31 moves e3d4 h8h6 d4e3 The old code evaluated it as "cp 0", the new code evaluation is around "cp -50" which is adequate. Brings 0.5-1 ELO gain. Passes [-3.00,1.00]. STC: http://tests.stockfishchess.org/tests/view/584ece040ebc5903140c5aea LLR: 2.96 (-2.94,2.94) [-3.00,1.00] Total: 47744 W: 8537 L: 8461 D: 30746 LTC: http://tests.stockfishchess.org/tests/view/584f134d0ebc5903140c5b37 LLR: 2.96 (-2.94,2.94) [-3.00,1.00] Total: 36775 W: 4739 L: 4639 D: 27397 Patch has been rewritten into current form for simplification and logic slightly changed so that return a draw score if the position repeats once earlier but after or at the root, or repeats twice strictly before the root. In its original form, repetition at root was not returned as an immediate draw. After retestimng testing both version with SPRT[-3, 1], both passed succesfully, but this version was chosen becuase more natural. There is an argument about MultiPV in which an extended draw at root may be sensible. See discussion here: #925 For documentation, current version passed both at STC and LTC: STC LLR: 2.96 (-2.94,2.94) [-3.00,1.00] Total: 51562 W: 9314 L: 9245 D: 33003 LTC LLR: 2.96 (-2.94,2.94) [-3.00,1.00] Total: 115663 W: 14904 L: 14906 D: 85853 bench: 5468995
Merged with 881a9df |
@mcostalba, I've created an issue where I've tried to explain my point with an example position: #948. |
Implement a threefold repetition detection. Below are the examples of problems fixed by this change.
Loosing move in a drawn position.
position fen 8/k7/3p4/p2P1p2/P2P1P2/8/8/K7 w - - 0 1 moves a1a2 a7a8 a2a1
The old code suggested a loosing move "bestmove a8a7", the new code suggests "bestmove a8b7" leading to a draw.
Incorrect evaluation (happened in a real game in TCEC Season 9).
position fen 4rbkr/1q3pp1/b3pn2/7p/1pN5/1P1BBP1P/P1R2QP1/3R2K1 w - - 5 31 moves e3d4 h8h6 d4e3
The old code evaluated it as "cp 0", the new code evaluation is around "cp -50" which is adequate.
Brings 0.5-1 ELO gain. Passes [-3.00,1.00].
STC: http://tests.stockfishchess.org/tests/view/584ece040ebc5903140c5aea
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 47744 W: 8537 L: 8461 D: 30746
LTC: http://tests.stockfishchess.org/tests/view/584f134d0ebc5903140c5b37
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 36775 W: 4739 L: 4639 D: 27397