Refactor get_best_thread

[peregrineshahin]

Refactors get_best_thread function logic for readability

Passed non-reg STC:
https://tests.stockfishchess.org/tests/view/65a91c6679aa8af82b975500
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 186000 W: 46379 L: 46325 D: 93296
Ptnml(0-2): 269, 21374, 49634, 21480, 243

no functional change

[mstembera]

Sorry for the late review but I believe line 264 if (newThreadInProvenLoss && newThreadScore < bestThreadScore) didn't get ported correctly. To preserve the original logic I think it should be if (newThreadScore != -VALUE_INFINITE && newThreadScore < bestThreadScore). Do you agree?

[peregrineshahin]

Sorry for the late review but I believe line 264 if (newThreadInProvenLoss && newThreadScore < bestThreadScore) didn't get ported correctly. To preserve the original logic I think it should be if (newThreadScore != -VALUE_INFINITE && newThreadScore < bestThreadScore). Do you agree?

It's a pre-calculated boolean so it should be exactly the same, since bestThread is in proven loss the newThread can't be < and not negative infinity unless it's also in proven loss.

[mstembera]

I c ok that makes sense. What I now realize is that I don't understand why we want to select the new thread when newThreadScore < bestThreadScore ? We should want the opposite to maximize the score. Turns out this logic got reversed in #4990 which I believe is a bug. We want to stave off the mate the longest not shortest.

[peregrineshahin]

It's the other way around, the bug was that we were staving off the mated in scores.. currently no non proven mated score can be reported by any thread which wasn't the case before. This means that staving off mated in scores was neccessary because threads will stuck with a non proven mated in scores. If we use the previous iteration to prove mated in scores then a lower score has the correct best move anyways.

[mstembera]

It's the other way around, the bug was that we were staving off the mated in scores..

Hmm It seems that we should be staving off mate. Doesn't a more negative score mean the mate is closer so therefore we want a higher(less negative) score? Why was that a bug?

currently no non proven mated score can be reported by any thread which wasn't the case before. This means that staving off mated in scores was neccessary because threads will stuck with a non proven mated in scores. If we use the previous iteration to prove mated in scores then a lower score has the correct best move anyways.

What does it mean for a mate to be proven and why would it change the fact that when we are defending we want the longest as opposed to the shortest mate? That should still correspond to the least negative score as it did before.

Am I'm missing something fundamental about how this works?