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

For rootComplexity eval is called even if in check #4512

Closed
vondele opened this issue Apr 9, 2023 · 5 comments
Closed

For rootComplexity eval is called even if in check #4512

vondele opened this issue Apr 9, 2023 · 5 comments

Comments

@vondele
Copy link
Member

vondele commented Apr 9, 2023

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 added a commit to dubslow/Stockfish that referenced this issue Apr 9, 2023
@dubslow
Copy link
Contributor

dubslow commented Apr 9, 2023

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
Copy link
Contributor

dubslow commented Apr 9, 2023

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.

vondele added a commit to vondele/Stockfish that referenced this issue Apr 9, 2023
The calculation of rootComplexity can't call eval when in check.
Doing so triggers an assert if compiled in debug mode when
the rootpos is evaluated using classical eval.

Fixes official-stockfish#4512

Passed STC:
https://tests.stockfishchess.org/tests/view/6432697431feee5c6d306876
LLR: 2.93 (-2.94,2.94) <-1.75,0.25>
Total: 41096 W: 11017 L: 10815 D: 19264
Ptnml(0-2): 113, 4172, 11780, 4366, 117

Passed LTC:
https://tests.stockfishchess.org/tests/view/6432974d31feee5c6d306fc0
...

No functional change
vondele added a commit to vondele/Stockfish that referenced this issue Apr 9, 2023
The calculation of rootComplexity can't call eval when in check.
Doing so triggers an assert if compiled in debug mode when
the rootpos is evaluated using classical eval.

Fixes official-stockfish#4512

Passed STC:
https://tests.stockfishchess.org/tests/view/6432697431feee5c6d306876
LLR: 2.93 (-2.94,2.94) <-1.75,0.25>
Total: 41096 W: 11017 L: 10815 D: 19264
Ptnml(0-2): 113, 4172, 11780, 4366, 117

Passed LTC:
https://tests.stockfishchess.org/tests/view/6432974d31feee5c6d306fc0
...

No functional change
@dubslow
Copy link
Contributor

dubslow commented Apr 9, 2023

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
Copy link
Member Author

vondele commented Apr 9, 2023

I'll add the additional assert in evaluate.

@dubslow
Copy link
Contributor

dubslow commented Apr 9, 2023

I was just about to PR this lol master...dubslow:Stockfish:inCheckAssert

@vondele vondele closed this as completed in b36d39d Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants