do not change cursed wins to draws #5805
Conversation
|
clang-format 18 needs to be run on this PR. (execution 12855041177 / attempt 1) |
|
Master: Patch: |
|
Passed the matetrack PV verification cleanly: |
|
As reported in vondele/matetrack#130 (comment) this patch seems to uncover a subtle issue with the pv extension in the absence of the 50mr. For example: So a TBloss score at root comes with a PV that after 50 moves gives a drawn position. I believe this may have to do with the rank calculation in So I would suggest to merge this PR as a bug-fix, and then deal with the incorrect PVs in a follow-up. |
| @@ -1989,7 +1990,7 @@ void syzygy_extend_pv(const OptionsMap& options, | |||
| pos.do_move(pvMove, st); | |||
|
|
|||
| // Do not allow for repetitions or drawing moves along the PV in TB regime | |||
| if (config.rootInTB && pos.is_draw(ply)) | |||
| if (config.rootInTB && ((rule50 && pos.is_draw(ply)) || pos.has_repeated())) | |||
There was a problem hiding this comment.
pos.is_draw(ply) and pos.has_repeated() are different in how they flag 2 and 3 fold repetitions. However, it might be that pos.has_repeated() but that needs some motivation.
There was a problem hiding this comment.
Yes, I tried minimal changes at first, and did not want to add a new function to the code base at this stage. We may eventually have to, but I do not know.
| @@ -2008,7 +2009,7 @@ void syzygy_extend_pv(const OptionsMap& options, | |||
| // Step 2, now extend the PV to mate, as if the user explored syzygy-tables.info | |||
| // using top ranked moves (minimal DTZ), which gives optimal mates only for simple | |||
| // endgames e.g. KRvK. | |||
| while (!pos.is_draw(0)) | |||
| while (!((rule50 && pos.is_draw(0)) || pos.has_repeated())) | |||
| @@ -2061,7 +2062,7 @@ void syzygy_extend_pv(const OptionsMap& options, | |||
| // We adjust the score to match the found PV. Note that a TB loss score can be | |||
| // displayed if the engine did not find a drawing move yet, but eventually search | |||
| // will figure it out (e.g. 1kq5/q2r4/5K2/8/8/8/8/7Q w - - 96 1 ) | |||
| if (pos.is_draw(0)) | |||
| if (rule50 && pos.is_draw(0)) | |||
There was a problem hiding this comment.
Also here not necessarily equivalent with 3-folds, not sure we can have that case.
There was a problem hiding this comment.
Since pos.is_draw(0) would flag any position after 50moves without capture or pawn move as draw, we have to guard this here by rule50 to avoid cursed wins to be changed to draws. I believe this is the only place that is strictly necessary to fix the bug. The other changes try to maintain the nice PV extension also for !rule50, without success so far it seems.
|
It would be good to get the correct PVs as well of course. Might need some additional thinking about what is causing the issue. |
Agreed. It seems to me by now that this PR only deals with a symptom, whereas the cause are the incorrect PVs. I.e. somehow when 50mr is off, the extension includes positions in the PV that are TB draws. And those incorrect extensions then lead to the score change at root. I suspect the reason that the extended PVs can have drawn positions is somehow hidden in here: // Better moves are ranked higher. Certain wins are ranked equally.
// Losing moves are ranked equally unless a 50-move draw is in sight.
int r = dtz > 0 ? (dtz + cnt50 <= 99 && !rep ? MAX_DTZ - (rankDTZ ? dtz : 0)
: MAX_DTZ / 2 - (dtz + cnt50))
: dtz < 0 ? (-dtz * 2 + cnt50 < 100 ? -MAX_DTZ - (rankDTZ ? dtz : 0)
: -MAX_DTZ / 2 + (-dtz + cnt50))
: 0;But I do not see it. :) |
|
Closing in favour of #5814. |
This fixes an oversight introduced in #5414.
The problem is that
is_draw()has no knowledge of the UCI optionSyzygy50MoveRule, and so its calls have to be guarded insyzygy_extend_pv().An example position where the bug triggers is
7b/8/6b1/8/5Nk1/4K3/8/8 b - - 0 1.A review by @vondele would be good.
No functional change.