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

Remove nmpMinPly #5586

Closed
wants to merge 1 commit into from
Closed

Conversation

TarasVuk
Copy link
Contributor

Passed STC:
LLR: 2.95 (-2.94,2.94) <-1.75,0.25>
Total: 108256 W: 28116 L: 27975 D: 52165
Ptnml(0-2): 394, 12286, 28608, 12465, 375
https://tests.stockfishchess.org/tests/view/66d5ffa99de3e7f9b33d14bd

Passed LTC:
LLR: 2.95 (-2.94,2.94) <-1.75,0.25>
Total: 184530 W: 46414 L: 46360 D: 91756
Ptnml(0-2): 152, 20203, 51499, 20261, 150
https://tests.stockfishchess.org/tests/view/66d6f6219de3e7f9b33d1562

bench: 1281840

Copy link

clang-format 18 needs to be run on this PR.
If you do not have clang-format installed, the maintainer will run it when merging.
For the exact version please see https://packages.ubuntu.com/noble/clang-format-18.

(execution 10816070910 / attempt 1)

@peregrineshahin
Copy link
Contributor

It would be nice to understand if this is a looser form of NMP or a stricter one.. not so clear at first sight..

@MinetaS
Copy link
Contributor

MinetaS commented Sep 11, 2024

matetrack result:

Engine ID:     Stockfish dev-20240911-5a07e893
Total FENs:    6554
Found mates:   3161
Best mates:    2198

@xu-shawn
Copy link
Contributor

It would be nice to understand if this is a looser form of NMP or a stricter one.. not so clear at first sight..

Should be looser since it removes the disabling of NMP on plies near the verification search root. So idealy this should be tested with the Zugzwang test suite https://github.com/official-stockfish/fishtest/wiki/Creating-my-first-test#useful-resources

@MinetaS
Copy link
Contributor

MinetaS commented Sep 12, 2024

Tested with Threads=1, Hash=1024 and go nodes 1000000000. Positions where PR took nodes >= 10M:

FEN Bestmove PR master
k2N2K1/8/8/8/5R2/3n4/3p4/8 w - - 0 1 Rf7 11.04M nodes 51.69M nodes
8/6B1/p5p1/Pp4kp/1P5r/5P1Q/4q1PK/8 w - - 0 1 Qxh4+ 726M nodes 63.26M nodes
n1QBq1k1/5p1p/5KP1/p7/8/8/8/8 w - - 0 1 Bc7 Not found 864M nodes
8/8/8/1B6/6p1/8/4KPpp/3N2kr w - - 0 1 Kd3/Ke3 Not found 60k nodes
8/8/8/1B6/6p1/8/3K1Ppp/3N2kr w - - 0 1 f4 731M nodes 41.95M nodes
8/8/8/2p5/1pp5/brpp4/1pprp2P/qnkbK3 w - - 0 1 h3 Not found, D245 1.76M nodes
1k3b1q/pP2p1p1/P1K1P1Pp/7P/2B5/8/8/8 w - - 0 1 Kd5/Bb5 Not found Not found

@peregrineshahin
Copy link
Contributor

peregrineshahin commented Sep 12, 2024

Oops you have Not found, D245 an irreversible mistake here??
Could you please check if that is the case with master?

@peregrineshahin
Copy link
Contributor

sorry I didn't see the master column, yeah this patch introduces a point of no return.

@MinetaS
Copy link
Contributor

MinetaS commented Sep 12, 2024

Oops you have Not found, D245 an irreversible mistake here??

For the specific position depth 245 was reached before 1B nodes and the bestmove was not found.

info depth 245 seldepth 22 multipv 1 score cp 0 nodes 1545651 nps 2851754 hashfull 1 tbhits 0 time 542 pv h2h4 a1a2 h4h5 a2a1 h5h6 a1a2 h6h7 a2a1 h7h8r a1a2 h8h1 a2a1
bestmove h2h4 ponder a1a2

@peregrineshahin
Copy link
Contributor

very undesirable sitiuation, to keep falling in the same search heuristic trap over and over again, until reaching max depth, and halting the engine..

@MinetaS
Copy link
Contributor

MinetaS commented Sep 12, 2024

Well these positions are a small portion of the given epd. I would not make a conclusion with few test cases, still it needs analysis to figure out if this PR actually has regression on zugzwang positions.

@peregrineshahin
Copy link
Contributor

peregrineshahin commented Sep 12, 2024

Well these positions are a small portion of the given epd. I would not make a conclusion with few test cases, still it needs analysis to figure out if this PR actually has regression on zugzwang positions.

note here the conversation
https://discord.com/channels/435943710472011776/813919248455827515/1208072882178105364
It is the only example where we know that the problem is irrevirsible
No need for sample size weighting when you know exactly where the problem is from a billion of calls and repeated failure on this positions.
the counter argument should be to find a such position where the mistake can't be undone with master.

@peregrineshahin
Copy link
Contributor

Well,I might be exaggerating but in principle I don't see any reason to have the verification search if not for irreversible mistakes.

@Disservin
Copy link
Member

Since null move pr's pop up every now and then I propose we should find a solution on how we treat those, and what tests they have to pass in order to be considered for merging. We seem to have that discussion on a lot of these pr's with no clear path forward. If someone likes to propose something please open a discussion/issue.

I'll close this one in the meantime.

@Disservin Disservin closed this Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants