The latest regression tests show that we lost Elo in a couple of variants.
I had a look at the merged upstream changes and found that during this merge the passed pawn array was updated for standard chess, but not for the variants. Since the translation from rank to array index was shifted by one, this might explain why there is a regression in variants that rely on passed pawn evaluation, such as horde and atomic.
I think we should do one or two tests to check whether fixing that regains roughly the same amount of Elo to be sure that we did not introduce a second regression somewhere, since there are other upstream changes that might also affect playing strength for variants, such as king safety changes and changes to the null move verification search.
Oops...
Could we not classify this mis-merge (off-by-one logic error) itself as a bug, fix it, then conduct regression tests with the fixed master? I think it's worth fixing unless doing so loses Elo.
Yes, we can also do the testing that way. I would anyway suggest to apply the fix regardless of whether it loses Elo and only if it does so for a variant revert the fix as a "parameter tweak". Otherwise it would later be confusing how the (accidental) improvement made it into the code.
That approach sounds reasonable & good to me.
I'm unsure whether to stop http://35.161.250.236:6543/tests/view/5a7278d76e23db0fbab0d341 in order to rebase d7728b472c293a4b97a202ba9b31fb224ba76f94 due to ~70 lost Horde Elo without 8b11863586119996960d6462bb38efcc29bdb4c6.
In theory either bug could cause enough noise (random or otherwise) that could unpredictably bias testing of the other bugfix. :-/
I would be ok with either approach. On the one hand I think the two fixes should not interfere that much, on the other hand rebasing after a significant change in playing strength usually makes sense.
Thanks, that makes sense.
I see 4d648522c5818e8460efec030fadafb101db50e1 tweaked Seed[HORDE_VARIANT][0] although the actual value is unused (because upstream doesn't expect 1st-rank pawns). Adding connected/phalanx bonuses for 1st-rank pawns (which removes lines of code, but could require parameter tweaks to not regress), while possibly interesting, isn't regression-related...
Upstream redefined how doubled is computed prior to 8e094cd5c3f246f2212f1f3b72fac6c0f765d326 so there isn't a need to re-test / attempt to simplify it away.
Now I'm curious (given the two fixes) how much of a regression remains since Dec 13:
http://35.161.250.236:6543/tests/view/5a7469836e23db0fbab0d35a
For completeness' sake we should test (as previously discussed) the off-by-one error bugfix (I'm starting with horde, not entirely sure what bounds to use):
http://35.161.250.236:6543/tests/view/5a746b516e23db0fbab0d35c
Since Niklas made a new fishnet release, I would suggest to use the fishnet release tags for regression tests, since I think it is useful to directly see the tags of the versions that were compared in a regression test, e.g., when accessing them via the regression test page.
I agree. These tests reveal that only Racing Kings variant has regressed, and by <20 Elo!
ELO: -8.34 +-10.9 (95%) LOS: 6.6%
Total: 2000 W: 487 L: 535 D: 978
http://35.161.250.236:6543/tests/view/5a7470796e23db0fbab0d36a
Yes, the new results look good, so I am closing this.
Most helpful comment
Yes, the new results look good, so I am closing this.