Remove set statScore to zero

[silversolver1]

Simplification. Removes setting statScore to zero if negative.

STC:
LLR: 2.95 (-2.94,2.94) {-1.50,0.50}
Total: 84820 W: 16100 L: 16033 D: 52687
Ptnml(0-2): 1442, 9865, 19723, 9944, 1436
https://tests.stockfishchess.org/tests/view/5e654fdae42a5c3b3ca2e2f8

LTC:
LLR: 2.94 (-2.94,2.94) {-1.50,0.50}
Total: 57658 W: 7435 L: 7391 D: 42832
Ptnml(0-2): 441, 5397, 17104, 5451, 436
https://tests.stockfishchess.org/tests/view/5e657ce9e42a5c3b3ca2e307

Rationale: The idea was basically to not change values to something they're not, especially as doing so would be altering the value from its true state. Essentially: if the statScore was in fact negative, why would we treat it as if it were not?

Also, my first passed LTC! And hopefully my first successful PR to something on Github! :)

Bench 5204700

[silversolver1]

It looks like both the AppVeyor and Travis CI builds are having issues. Can anyone help me see what I'm doing wrong?

My bash file is

#!/bin/bash
# custommakefish.sh

cd Stockfish-master/src

arch_cpu=x86-64

make -j profile-build ARCH=${arch_cpu} COMP=mingw
strip stockfish.exe
mv stockfish.exe ../../stockfish_local_modified_${arch_cpu}.exe 
make clean
cd

This gives me Bench 5204700

For reference, I am on Windows and I utilized in essence the script from this guide for 64 bit Windows using the MSYS2 MinGW 64-bit shell: https://github.com/glinscott/fishtest/wiki/Building-stockfish-on-Windows

[vondele]

@silversolver1 the reference bench number is extracted from the commit message (not the PR comment). You could fix that for example with

git commit --allow-empty -m "Bench: 5204700"
git push origin no_zero_ss_neg

[silversolver1]

@vondele Thanks, I did what you said and it looks like the AppVeyor CI went fully through and 3/4 of the Travis checks as well.

It looks like the remaining Travis check failed because an assert in endgame.cpp was called?
abs(result) < VALUE_TB_WIN_IN_MAX_PLY

Not entirely sure why because I don't think what I removed touches that but presumably there's some interaction.

I'm not really sure how to proceed to be honest.

Any advice would be appreciated.

[vondele]

@silversolver1 interesting.. your patch changes search, and exposes what is presumably a recently introduced bug.

Nothing to worry about from your side, but @protonspring ;-)

On this position, we're making the eval somewhat to large:
position fen k7/8/K7/NB6/8/8/8/8 w - - 0 1
endgame.cpp:162: T Endgame<E, T>::operator()(const Position&) const [with EndgameCode E = KBNK; T = Value]: Assertion `abs(result) < VALUE_TB_WIN_IN_MAX_PLY' failed.

This might be accidentally fixed by #2577 ... let me have a look.

[protonspring]

The values in the Corners array were raised from 20-100 to 6k. The testing at the time demonstrated significant gains for KBNK games with no regression in master, but yes. 6k above VALUE_KNOWN_WIN is probably out of bounds.

The equations in #2577 closely represent the array values, so I wouldn't expect it to fix anything.

Perhaps for this specific ending, reduce the base so that adding the 6k approaches VALUE_KNOWN_WIN. Probably requires additional testing though to see what the ramifications are.

---

EDIT: I was confused between VALUE_KNOWN_WIN and VALUE_TB_IN_MAX_PLY.

[vondele]

@protonspring first quick testing with #2577 doesn't show this issue.... However, we have to be sure. Can you have a more careful look, determining the maximum value, and making sure it is below VALUE_TB_WIN_IN_MAX_PLY ?

[protonspring]

VALUE_TB_IN_MAX_PLY = (VALUE_MATE - 2 * MAX_PLY = (32000 - 512) = 31488
Absolute max from KBNK endgame less than: VALUE_KNOWN_WIN + 120 + 6400 = 16520.
I don't see any way that this endgame could exceed VALUE_TB_IN_MAX_PLY.

It's possible that we were going out of bounds somewhere on the arrays and the equations fix it?

[protonspring]

I poked around a bit. . the value of the result before dying was 677747157. Definitely seems like an out of bounds thing.

[vondele]

hmm mysterious indeed. For me I have the assert when I compile as:

make -j ARCH=x86-64-modern debug=yes optimize=yes build

but not

make -j ARCH=x86-64-modern debug=yes optimize=no build

that's for gcc 9.2.1. With clang++ both versions are clean.

The eval is also 47.89, well below the maximum, indeed.

[protonspring]

Oh. . this is the flip_square thing. ~loserKsq is wrong. This was fixed in the simplifications, but it's not commited to master yet.

[protonspring]

~loserKsq should be flip_rank(loserKsq). See #2567

[vondele]

I see. That's consistent with the compiler variations, and indeed out of bounds. Also means, this code is indeed not exercised in bench (enough), otherwise this mistake would have been noticed before. Could you have a careful look if any other operator~ passed through the cracks?

[protonspring]

Yes. . . after I missed the loserKsq, I took a pretty careful look. That was the only missing one, but I will look again.

@silversolver1 You could make the change I suggested and see if it fixes the build. I assume it will.

[vondele]

@protonspring, no need for @silversolver1 to do this. Let me take care of as part of the merging of the other PR.

However, @silversolver1 you might want to update the AUTHORS file as part of this PR :-) or just drop your full name in this thread (unless you want to be there just with your github handle).

[vondele]

@silversolver1 master has changed meanwhile, so the bench has changed... don't worry I'll merge this shortly and take care.

[silversolver1]

@vondele

Ok sure, thanks for your help!

[vondele]

Thanks!

[31m059]

@silversolver1 Congratulations on your successful PR! :)

[silversolver1]

@31m059 Thanks! :) And also thanks for helping me better understand what counts as a simplification lol