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

Simplify Pre-Qsearch Extension Condition #5518

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

Conversation

xu-shawn
Copy link
Contributor

@xu-shawn xu-shawn commented Jul 26, 2024

Passed Non-regression STC:
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 47424 W: 12362 L: 12151 D: 22911
Ptnml(0-2): 180, 5489, 12178, 5670, 195
https://tests.stockfishchess.org/tests/view/669ffce14ff211be9d4ecb0d

Passed Non-regression LTC:
LLR: 3.04 (-2.94,2.94) <-1.75,0.25>
Total: 117372 W: 29675 L: 29545 D: 58152
Ptnml(0-2): 84, 12733, 32941, 12825, 103
https://tests.stockfishchess.org/tests/view/66a0eaa34ff211be9d4ecbcd

bench 1660869

@snicolet
Copy link
Member

snicolet commented Jul 27, 2024

This has the risk to make unbounded search extensions if the transposition table contains the pv line? We have the same guard in step 15.

@dubslow
Copy link
Contributor

dubslow commented Jul 27, 2024

Indeed, this strikes me as the sort of thing which wasn't for Elo but for minimizing explosions. So long as deleting it doesn't gain elo, probably better to keep it (this has considerable historical precedent)

@peregrineshahin
Copy link
Contributor

This has the risk to make unbounded search extensions if the transposition table contains the pv line?

I don't honestly see what's unbounded about a condition that depends on ttmove existing, also on the other hand, one would argue why not, what makes it a risk specifically?
If the PV line was in the transposition table, wouldn't it makes sense to at least search the PV line further?
Doesn't this make us confident about our PV lines, which is already the goal?

@xu-shawn
Copy link
Contributor Author

I don't think search explosion is of concern here because the next ply is searched only at a depth of 1. This means that pruning is aggressive and most of the child nodes dive in to qsearch immediately. At the very worst case such an explosion will probably only result in a few thousand qsearch calls, which is not really enough to be considered as a search explosion.

@xu-shawn
Copy link
Contributor Author

xu-shawn commented Jul 27, 2024

Did a depth 22 bench on big bench, no explosions

                if (move == ttData.move)
+               {
+                   dbg_extremes_of(ss->ply);
                    newDepth = std::max(newDepth, 1);
+               }
❯ ./stockfish bench 256 1 22 varied_1000.epd
...
Extremity #0: Total 2599276 Min 0 Max 60

===========================
Total time (ms) : 1141079
Nodes searched  : 1001919955
Nodes/second    : 878046

@snicolet
Copy link
Member

Also note that you are simplifying an Elo gaining patch which has been committed a few hours ago:

  1. Are we sure the "simpler" version would have been an Elo gain against master? This test suggest not: https://tests.stockfishchess.org/tests/view/669a317e4ff211be9d4ec50b

  2. We usually give Elo gaining patches a few days of rest to permit the surrounding parameters to adapt to the new code.

@xu-shawn
Copy link
Contributor Author

xu-shawn commented Jul 27, 2024

  1. Are we sure the "simpler" version would have been an Elo gain against master? This test suggest not: https://tests.stockfishchess.org/tests/view/669a317e4ff211be9d4ec50b

The test was stopped prematurely, it's not statistically rigorous to make conclusions from it.

  1. We usually give Elo gaining patches a few days of rest to permit the surrounding parameters to adapt to the new code.

I don't see how this simplification would have any interaction with any other parameters, especially since ss->ply > thisThread->rootDepth * 2 triggers in only 0.01% of pre-qsearch extensions.

@peregrineshahin
Copy link
Contributor

Well one concern is that if one is allowed to do this *2 *3 *4 while nothing already triggers and they means nothing, then the patch already was phacking the pass.. So...

@xu-shawn
Copy link
Contributor Author

rebased

@MinetaS
Copy link
Contributor

MinetaS commented Jul 29, 2024

matetrack result:

Engine ID:     Stockfish dev-20240728-0a8ae248
Total FENs:    6554
Found mates:   3282
Best mates:    2340

@Disservin
Copy link
Member

mh i think id like to keep this for now given that we will soonish release sf 17 but yea the extend to which we extend the search depth here is rather limited and constrained

@xu-shawn
Copy link
Contributor Author

Rebased

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

Successfully merging this pull request may close these issues.

6 participants