search

official-stockfish / Stockfish

A free and strong UCI chess engine
https://stockfishchess.org/
GNU General Public License v3.0
11.76k stars 2.3k forks source link

For rootComplexity eval is called even if in check #4512

Closed vondele closed 1 year ago

vondele commented 1 year ago

Describe the issue

causes assert with for example ./stockfish*.exe bench 16 1 13 default depth classical

Expected behavior

no assert triggered

Steps to reproduce

see above

Anything else?

No response

Operating system

All

Stockfish version

master

dubslow commented 1 year ago

I find Takes 1 and 3 to be rather illogical.

Take 1 results in a base time factor at the minimum of 0.85, changing the TM when in check, which is unnatural but might pass nonregression; Take 3 is unnecessarily complicated, appearing to be Take 2 bug fix plus a separate logic change intended to be a gainer.

Take 2, on the other hand, is the logical and natural fix: keep the current effect as is when not in check, and when in check, simply set the time factor to the non-effect value of 1.0. This is also more or less what I wrote and submitted. On this basis, I paused my test as duplicate of your Take 2. (In fact, your Take 2 is actually better than mine, since it fixes a second problem in master where we don't clear the mainThread complexity in ucinewgame.)

I recommend also pausing Take 1. Take 3 is interesting, I suppose, but in general I would prefer to keep bugfixes and gainers in separate patches and separate tests.

dubslow commented 1 year ago

On a deeper note, this suggests that the current default bench does not include any classical in check positions, i.e. doesn't offer sufficient code coverage from a test perspective. I suggest that, in addition to fixing this immediate issue, we upgrade the bench to include this check in the future.

dubslow commented 1 year ago

4515 LGTM (presuming passer).

On the question of preventing this in the future, we could certainly make that in check position be HCE in the default mixed bench, but is it perhaps more useful to instead add the same assert to nnue eval as well? Or is that better left alone?

vondele commented 1 year ago

I'll add the additional assert in evaluate.

dubslow commented 1 year ago

I was just about to PR this lol https://github.com/official-stockfish/Stockfish/compare/master...dubslow:Stockfish:inCheckAssert