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

Fix issues from using adjustedDepth too broadly #1792

Closed
wants to merge 1 commit into from

Conversation

vondele
Copy link
Member

@vondele vondele commented Oct 28, 2018

the committed FH patch (081af90) had a number of changes beyond adjusting the depth of search on fail high, with some undesirable side effects.

  1. decreasing depth on PV output, confusing GUIs and players alike as described in issue PV depth increasing and decreasing (sorting bug?) #1787. The depth printed is anyway a convention, let's consider adjustedDepth an implementation detail, and continue to print rootDepth. Depth, nodes, time and move quality all increase as we compute more. (fixing this output has no effect on play).

  2. fixes go depth output (now based on rootDepth again, no effect on play), also reported in issue PV depth increasing and decreasing (sorting bug?) #1787

  3. lastBestDepth is used to compute how long a move is stable, a new move found during FH is incorrectly considered stable if based on adjustedDepth instead of rootDepth. (Changes TM). Reverting this:
    passed STC [-3,1]
    LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
    Total: 82982 W: 17810 L: 17808 D: 47364
    http://tests.stockfishchess.org/tests/view/5bd391a80ebc595e0ae1e993

passed LTC [-3,1]
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 109083 W: 17602 L: 17619 D: 73862
http://tests.stockfishchess.org/tests/view/5bd40c820ebc595e0ae1f1fb

  1. In the thread voting scheme, the rank of the FH thread is now artificially low, incorrectly since the quality of the move is much better than what adjustedDepth suggests (e.g. if it takes 10 iterations to find VALUE_KNOWN_WIN, it has very low depth). Further evidence comes from a test that showed that the move of highest depth is not better than that of the last PV (which is potentially of much lower adjustedDepth).
    I.e. this test http://tests.stockfishchess.org/tests/view/5bd37a120ebc595e0ae1e7c3 failed SPRT [0, 5]
    LLR: -2.95 (-2.94,2.94) [0.00,5.00]
    Total: 10609 W: 2266 L: 2345 D: 5998

In a running 5+0.05 th 8 test (more than 10000 games) a positive Elo estimate is shown (strong enough for a [-3,1], possibly not [0,4]):
http://tests.stockfishchess.org/tests/view/5bd421be0ebc595e0ae1f315
LLR: -0.13 (-2.94,2.94) [0.00,4.00]
Total: 13644 W: 2573 L: 2532 D: 8539
Elo 1.04 [-2.52,4.61] / LOS 71%

Thus, restore old behavior as a bugfix, keeping the core of the FH resolving scheme. (This is non-functional for a bench, but changes searches via TM and in the threaded case).

Bench: 3166402

the committed FH patch (081af90) had a number of changes beyond adjusting the depth of search on fail high, with some undesirable side effects.

1) decreasing depth on PV output, confusing GUIs and players alike as described in issue official-stockfish#1787. The depth printed is anyway a convention, let's consider adjustedDepth an implementation detail, and continue to print rootDepth. Depth, nodes, time and move quality all increase as we compute more. (fixing this output has no effect on play).

2) fixes go depth output (now based on rootDepth again, no effect on play), also reported in issue official-stockfish#1787

3) lastBestDepth is used to compute how long a move is stable, a new move found during FH is incorrectly considered stable if based on adjustedDepth instead of rootDepth. (Changes TM). Reverting this:
passed STC [-3,1]
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 82982 W: 17810 L: 17808 D: 47364
http://tests.stockfishchess.org/tests/view/5bd391a80ebc595e0ae1e993

passed LTC [-3,1]
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 109083 W: 17602 L: 17619 D: 73862
http://tests.stockfishchess.org/tests/view/5bd40c820ebc595e0ae1f1fb

4) In the thread voting scheme, the rank of the FH thread is now artificially low, incorrectly since the quality of the move is much better than what adjustedDepth suggests (e.g. if it takes 10 iterations to find VALUE_KNOWN_WIN, it has very low depth). Further evidence comes from a test that showed that the move of highest depth is not better than that of the last PV (which is potentially of much lower adjustedDepth).
I.e. this test http://tests.stockfishchess.org/tests/view/5bd37a120ebc595e0ae1e7c3 failed SPRT [0, 5]
LLR: -2.95 (-2.94,2.94) [0.00,5.00]
Total: 10609 W: 2266 L: 2345 D: 5998

In a running 5+0.05 th 8 test (more than 10000 games) a positive Elo estimate is shown (strong enough for a [-3,1], possibly not [0,4]):
http://tests.stockfishchess.org/tests/view/5bd421be0ebc595e0ae1f315
LLR: -0.13 (-2.94,2.94) [0.00,4.00]
Total: 13644 W: 2573 L: 2532 D: 8539
Elo	1.04 [-2.52,4.61] / LOS 71%

Thus, restore old behavior as a bugfix, keeping the core of the FH resolving scheme. (This is non-functional for a bench, but changes searches via TM and in the threaded case).

Bench: 3166402
@snicolet
Copy link
Member

This pull request has four points 1-4.

• I had already considered point 1, and I must say that I like it because it fixes the "strange" behavior of non-increasing reported depths which has puzzled users in issue #1787. The searched depth in that case case can be seen as a mixture of adjustedDepth and rootDepth, since the search will be nominally at adjustedDepth but using information at rootDepth in the hash table for some internal cuts.

• Points 2-4 I have not studied yet about, will have to check the code carefully before giving opinion.

Thanks for the PR and the tests! :-)

@vondele
Copy link
Member Author

vondele commented Oct 30, 2018

This should also fix #1791 which is kind of a duplicate of #1787.

Let me know if you need more explanation for points 2-4.

@pb00068
Copy link
Contributor

pb00068 commented Oct 31, 2018

Sorry to say, but I'm not really convinced of point 3.
Although we have positions deeper analyzed with rootDepth in TT, these are with obsolete bounds and therefore of little avail. adjustedDepth is the effective and real depth used in last search, so if we want preserve our timeReduction logic, then this is the proper variable to take into consideration IMO.
If point 3 really fixes something, I would have expected at least a mild effect on playing strength: this wasn't measured.

P.S.: Anyhow I'm pretty neutral about point 3 since it seems not having an effect at all.
So in ensemble I'm fine with this patch.
At the begin I preferred to return adjustedDepth since it looked more correct and sincere, but the complications deriving from this are simply to multiple for being resolved in an alternative easy and convincing way.

@vondele
Copy link
Member Author

vondele commented Oct 31, 2018

@pb00068 nice to read that you're fine with the patch.

Since I was not clear with point 3. Let me give an example for current master. If at rootDepth 20 but adjustedDepth 5 a new move is found (lastBestMoveDepth=5), and this move is still the best at rootDepth=completedDepth=adjustedDepth= 21, we now give a large timeReduction bonus (because the move is incorrectly considered as stable since 16 iterations). Fortunately, this seems rare indeed.

@pb00068
Copy link
Contributor

pb00068 commented Oct 31, 2018

@vondele Ahh, now I have understood. Thanks for clarification!

@snicolet
Copy link
Member

snicolet commented Nov 1, 2018

Merged via 3f1eb85, thanks a lot!

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.

None yet

3 participants