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 nmpColor #4511
Remove nmpColor #4511
Conversation
STC: https://tests.stockfishchess.org/tests/view/642e293977ff3301150e9b55 LLR: 2.94 (-2.94,2.94) <-1.75,0.25> Total: 161848 W: 43332 L: 43254 D: 75262 Ptnml(0-2): 418, 16987, 46058, 17021, 440 LTC: https://tests.stockfishchess.org/tests/view/642fea9420eb941419bde296 LLR: 2.95 (-2.94,2.94) <-1.75,0.25> Total: 120208 W: 32529 L: 32418 D: 55261 Ptnml(0-2): 35, 11424, 37080, 11525, 40 bench 3672914
I think we need to know if this is even a functional change then if it is test some zugzwang positions probably. |
I assume that it's functional, just not in the bench positions because bench depth is low and because bench doesn't have positions which contain zugzwangs, which are quite rare indeed from a practical point of view. Obviously, if it isn't functional, then this is dead code anyways, so let's assume it is functional. The effect of this patch is to do stricter verification searches. In master, when performing a verification search, we actually do allow the opposing player to do null move pruning, i.e. we assume that if one side is in zugzwang in a given position, then the opponent surely cannot be in zugzwang a few ply down the road. Removing this code removes this assumption, and thus actually catches more zugzwangs than master. On that basis, I think it's fine to merge as-is, tho if someone were to bother to test the patch on a few zugzwangs, I certainly wouldn't mind seeing the data. |
Okay makes sense, I think now this going in the correct direction, less nmp (if possible) is indeed a preferable outcome in general. |
There is no need to assume it is functional. You can run a deep bench, and see that it is functional. As others have expressed, there is some reluctance to remove NMP verification code, this was added for a reason. I think the old PRs that introduced this code had example FENs. It would be interesting to see how it performs. |
Well then it's a good thing this patch does the opposite and does more verification (indeed removing verification altogether also passed nonreg, but the consensus is that that's not worth submitting) |
Ah right, mixed the two patches |
I can do some verify with the extended Zugzwang-Allgeuer.epd which I used when introducing nmpColor
|
Intuitively this should make verification search better. And if it costs no elo it's all good. |
As I expected master and patch do perform equally on Zugzwang-Allgeuer.epd (singlecore, 256m hash, max. 25 seconds per problem) Interesting is that patch solves systematically problem nr. 23 (8/8/8/1B6/6p1/8/3K1Ppp/3N2kr w - - bm f4) Here the report of the analysys mady using Arena 3.5.1:
|
I made a deep bench ( bench 256 1 24) to measure how often a null-move now will get allowed for the other side to move in regard to current master. Result: So only in 0.09% of verification nodes we will now insert a null-move where current master does not. |
STC: https://tests.stockfishchess.org/tests/view/642e293977ff3301150e9b55
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 161848 W: 43332 L: 43254 D: 75262
Ptnml(0-2): 418, 16987, 46058, 17021, 440
LTC: https://tests.stockfishchess.org/tests/view/642fea9420eb941419bde296
LLR: 2.95 (-2.94,2.94) <-1.75,0.25>
Total: 120208 W: 32529 L: 32418 D: 55261
Ptnml(0-2): 35, 11424, 37080, 11525, 40
bench 3672914