--stockfish-- evaluation - #9
Conversation
📝 WalkthroughWalkthroughThe PR separates middlegame and endgame evaluation parameters, updates scoring and tuning logic, adds PGN pentanomial extraction, and restructures benchmark and SPSA workflows. ChangesEvaluation and benchmark pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant prepare_tools
participant test
participant evaluate
participant Ordo
participant stats_sprt
prepare_tools->>test: stage benchmark tools
test->>test: execute Fastchess shard matrix
test->>evaluate: upload shard PGNs and results
evaluate->>Ordo: merge PGNs and generate ratings
evaluate->>stats_sprt: calculate SPRT statistics
stats_sprt-->>evaluate: write sprt.txt
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tune_cmd.cpp (1)
746-775: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftGradient doesn't account for the new
sfandrule50damping in the forward pass.Lines 746-757 scale the score by
sf/64and by(100 - rule50)/100, butphase_mg/phase_egat Lines 771-772 only carry the phase split. For a pawnless ≤4-piece position the forward score is halved on the eg side while the eg gradient is still computed at full weight, and atrule50 = 100the forward score is 0 while the gradient is unchanged — the gradient no longer matches the loss it's supposed to descend.Folding the same factors into the phase weights keeps them consistent:
♻️ Proposed fix
- double phase_mg = comp.phase / 128.0; - double phase_eg = (128 - comp.phase) / 128.0; + double damp = (sf / 64.0) * ((100 - rule50) / 100.0); + double phase_mg = damp * comp.phase / 128.0; + double phase_eg = damp * (sf / 64.0) * (128 - comp.phase) / 128.0;(Note
sfappears twice on the eg path because it multipliescomp.egbefore the taper; adjust to taste, but the two paths need to agree.)Also worth noting:
eval_loss(Line 80) measures loss viaeval::eval(pos), while the gradient worker computes its ownscorehere. The line search and the gradient are therefore optimizing slightly different objectives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tune_cmd.cpp` around lines 746 - 775, Update the gradient calculation in the worker around accumulate_gradient so phase_mg and phase_eg include the same sf scaling and rule50 damping applied when computing v. Preserve the distinct mg/eg taper while ensuring both gradient paths match the forward score, including zero contribution when rule50 reaches 100. Also align eval_loss with the worker’s score calculation so line search and gradient optimize the same objective.
🧹 Nitpick comments (6)
spsa.py (2)
373-378: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGradient denominator ignores clamping.
plus/minusare clamped to[min, max]at Lines 355-358, butghatdivides by the nominal2 * ck[n] * deltas[n]. For a parameter resting on a bound the realized perturbation is asymmetric (or zero on one side), so the estimated gradient is inflated exactly where the parameter is least able to move. Using the actual realized difference(plus[n] - minus[n])as the denominator avoids this.- ghat = score / ( - 2 * ck[n] * deltas[n] - ) + spread = plus[n] - minus[n] + if abs(spread) < 1e-9: + continue + ghat = score / spread🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spsa.py` around lines 373 - 378, The SPSA gradient calculation in the parameter update must account for clamped perturbations. In the code producing ghat, replace the nominal denominator based on ck[n] and deltas[n] with the realized difference plus[n] - minus[n], while preserving the existing score numerator and value clamping behavior.
65-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSPRT is enabled inside the SPSA match template.
"enabled": Truemeans fastchess may stop an iteration's match early once an SPRT bound is reached, which truncates the sample and biasesf_plus/f_minusfor that iteration. For a fixed-budget gradient estimate you want allargs.pairsrounds played."sprt": { "alpha": 0.05, "beta": 0.05, "elo0": 0.0, "elo1": 5.0, "model": "normalized", - "enabled": True + "enabled": False },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spsa.py` around lines 65 - 72, Disable SPRT in the SPSA match template by changing the enabled setting in the "sprt" configuration used by the match setup. Ensure each iteration plays the full args.pairs budget so f_plus and f_minus are computed from complete samples.eval.cpp (1)
262-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
developedEgis now a no-op but still tuned.With Line 263 commented out,
developedEgno longer affects eval, yet it remains registered at Line 67 (TUNE(SetRange(1, 20), developedMg, SetRange(1, 20), developedEg)) and is emitted toWeights.h. SPSA/texel will spend budget perturbing a parameter with zero effect. Same applies to the endgame half ofearlyQueenPenalty(Lines 274-275), though that one shares a single parameter so it's harmless.Consider dropping
developedEgfrom theTUNElist while it's disabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eval.cpp` around lines 262 - 263, Remove developedEg from the TUNE registration so it is no longer tuned or emitted while its endgame evaluation contribution remains disabled. Leave the developedMg tuning and the commented-out evaluation line unchanged.tune_cmd.cpp (1)
135-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead
compute_game_phasecode
texel_tunenow gets the phase fromcomp.phase, andcompute_game_phasehas no call sites, so this helper, its constants, and any phase-masking code that depended on it are no longer reachable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tune_cmd.cpp` around lines 135 - 150, Remove the unused compute_game_phase helper and its associated phase constants from tune_cmd.cpp. Also remove any phase-masking or related code that depended exclusively on this helper, while preserving the existing comp.phase-based phase flow..github/workflows/games.yml (1)
112-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
fail-fast: trueon a 30-shard matrix cancels all in-progress shards on a single failure.A single flaky shard (network hiccup, one engine crash) will cancel the other 29 shards, wasting substantial already-completed game-play compute. Consider
fail-fast: falseso shard failures are isolated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/games.yml around lines 112 - 115, Update the matrix strategy containing the 30-entry shard list to set fail-fast to false, so a failure in one shard does not cancel other in-progress shards.extract_ptnml.py (1)
82-87: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueValidation is unreachable dead code.
scores[0]/scores[1]are always drawn from{-1, 0, 1}, so their sum is always in{-2, -1, 0, 1, 2}— exactlyPTNML_INDEX's keys. Thisraise RuntimeErrorcan never fire, despite the comment implying it verifies the pair is a genuine repeat (e.g., colors swapped). Consider removing this dead check or replacing it with an actual pairing-integrity check (e.g., confirmingWhite/Blackare swapped between the two games).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extract_ptnml.py` around lines 82 - 87, The check against PTNML_INDEX in the repeat-pair processing block is unreachable and does not validate pairing integrity. Remove it or replace it with a check that confirms the two games have swapped White/Black participants, while preserving the existing ptnml update and pairs_count increment for valid pairs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/games.yml:
- Around line 172-193: Update the SPRT workflow step and its artifact
configuration so they use a consistent output file: either redirect the SPRT
output to create sprt.txt, or remove the stale sprt.txt entry from the
upload-artifact paths. Preserve results.txt for the extracted and appended
results, and keep ratings.txt unchanged.
- Around line 133-142: Fix the fastchess command continuation in the workflow by
adding the missing space before the backslash after the -startup-ms,
-ucinewgame-ms, and -ping-ms arguments. Preserve the existing command structure
so the next line remains a separate -rounds argument.
In `@eval.cpp`:
- Around line 731-760: The tapered scoring pipeline is duplicated and handles
draw positions inconsistently. In eval.cpp lines 731-760, extract the complete
post-eval_components scoring, including the zero-component sentinel and K-vs-K
early returns, into a shared score_from_components helper and have eval() call
it; in tune_cmd.cpp lines 746-759, replace the inline scaling, tapering,
quantization, rule50, side-to-move, and tempo logic with that helper so both
paths are identical.
In `@extract_ptnml.py`:
- Around line 13-24: Update score_from_new to explicitly handle the "*" Result
before the win/loss branches, returning the appropriate neutral or non-counting
outcome required by the WDL/PTNML consumers instead of treating it as a loss;
preserve the existing decisive-result scoring and draw handling.
In `@spsa.py`:
- Around line 242-271: Update the subprocess.CalledProcessError handler around
the fastchess invocation to return the neutral `(0.5, 0.5)` result after logging
diagnostics, or re-raise the failure through existing handling so execution
cannot reach result-dependent logic without an assigned result. Preserve the
normal successful path in the surrounding evaluation function.
- Line 200: Update the row-parsing exception handling in load_params to
distinguish expected non-CSV UCI stdout lines from genuine malformed parameter
rows, reporting or propagating real parse failures instead of silently skipping
them and preserving a non-zero failure outcome. Also clean up the delete=False
temporary file created in the stdout-scraping fallback by unlinking it after the
recursive load_params call, including failure paths.
- Around line 349-367: Remove the initial unused deltas assignment and the
redundant plus_int/minus_int recomputation and unreachable equality guard around
the perturbation loop. In the gradient-update logic following this block, update
only parameters n where plus_int[n] differs from minus_int[n], while preserving
updates for parameters with an effective integer perturbation.
In `@tune.cpp`:
- Around line 79-80: Update the SPSA learning-gain value emitted by the tune.cpp
output so column 6 matches spsa.py’s local ak formula rather than Fishtest R_end
scaling. Ensure the generated gain is large enough for integer parameter
perturbations to change across 40 iterations, or reduce the stable_offset passed
to spsa.py accordingly.
---
Outside diff comments:
In `@tune_cmd.cpp`:
- Around line 746-775: Update the gradient calculation in the worker around
accumulate_gradient so phase_mg and phase_eg include the same sf scaling and
rule50 damping applied when computing v. Preserve the distinct mg/eg taper while
ensuring both gradient paths match the forward score, including zero
contribution when rule50 reaches 100. Also align eval_loss with the worker’s
score calculation so line search and gradient optimize the same objective.
---
Nitpick comments:
In @.github/workflows/games.yml:
- Around line 112-115: Update the matrix strategy containing the 30-entry shard
list to set fail-fast to false, so a failure in one shard does not cancel other
in-progress shards.
In `@eval.cpp`:
- Around line 262-263: Remove developedEg from the TUNE registration so it is no
longer tuned or emitted while its endgame evaluation contribution remains
disabled. Leave the developedMg tuning and the commented-out evaluation line
unchanged.
In `@extract_ptnml.py`:
- Around line 82-87: The check against PTNML_INDEX in the repeat-pair processing
block is unreachable and does not validate pairing integrity. Remove it or
replace it with a check that confirms the two games have swapped White/Black
participants, while preserving the existing ptnml update and pairs_count
increment for valid pairs.
In `@spsa.py`:
- Around line 373-378: The SPSA gradient calculation in the parameter update
must account for clamped perturbations. In the code producing ghat, replace the
nominal denominator based on ck[n] and deltas[n] with the realized difference
plus[n] - minus[n], while preserving the existing score numerator and value
clamping behavior.
- Around line 65-72: Disable SPRT in the SPSA match template by changing the
enabled setting in the "sprt" configuration used by the match setup. Ensure each
iteration plays the full args.pairs budget so f_plus and f_minus are computed
from complete samples.
In `@tune_cmd.cpp`:
- Around line 135-150: Remove the unused compute_game_phase helper and its
associated phase constants from tune_cmd.cpp. Also remove any phase-masking or
related code that depended exclusively on this helper, while preserving the
existing comp.phase-based phase flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f6a622c-3b32-4e0d-8a1c-82ecb9215b68
📒 Files selected for processing (11)
.github/workflows/games.yml.github/workflows/spsa.ymlWeights.heval.cppeval.hextract_ptnml.pymovepick.cppsearch.cppspsa.pytune.cpptune_cmd.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6da4c8915c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
eval.cpp (1)
251-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not treat a captured queen as early queen development.
The condition is true when the side has no queen, so queenless positions with undeveloped minors receive
earlyQueenPenalty. Require a queen to exist before treating its absence from the starting square as a move; mirror this predicate intune_cmd.cpp.Proposed fix
- if (!(board.pieces(QUEEN, c) & (1ULL << qStart))) { + Bitboard queens = board.pieces(QUEEN, c); + if (queens && !(queens & (1ULL << qStart))) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eval.cpp` around lines 251 - 275, Update the early queen development condition in the evaluation loop to first require that the side still has a queen, then check whether it is absent from its starting square before applying earlyQueenPenalty. Mirror the same queen-existence predicate in the corresponding tuning logic in tune_cmd.cpp.tune_cmd.cpp (1)
758-782: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDifferentiate the scorer used by the loss.
score_from_components()multiplies the raw score by(100 - rule50) / 100and returns a constant zero for the component sentinel, butcommon_factorignores both. This overstates gradients in rule-50 positions and updates parameters for forced-zero evaluations.Skip
accumulate_gradient()for the sentinel case and multiplycommon_factorby the rule-50 factor before accumulating gradients.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tune_cmd.cpp` around lines 758 - 782, Update the gradient path around score_from_components and accumulate_gradient: skip accumulation when the component sentinel produces a forced-zero score, and scale common_factor by the scorer’s rule-50 factor (100 - rule50) / 100 before calling accumulate_gradient. Preserve the existing loss calculation and other gradient behavior for non-sentinel positions.
🧹 Nitpick comments (1)
eval.cpp (1)
263-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
developedEgtuning parameter.
developedEgis still registered in theTUNElist, but this term no longer contributes to evaluation or gradients. Remove it from the tunable set, or restore its endgame contribution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eval.cpp` at line 263, Remove the unused developedEg parameter from the TUNE registration and any associated tuning declarations, since the developed-piece endgame term is disabled and no longer affects evaluation or gradients.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/games.yml:
- Line 138: Update the workflow command around the rounds argument to pass
github.event.inputs.rounds through an environment variable instead of
interpolating it into shell arithmetic. In the script, validate the value as a
decimal integer before calculating the per-shard rounds value, then use that
validated value for the command argument.
---
Outside diff comments:
In `@eval.cpp`:
- Around line 251-275: Update the early queen development condition in the
evaluation loop to first require that the side still has a queen, then check
whether it is absent from its starting square before applying earlyQueenPenalty.
Mirror the same queen-existence predicate in the corresponding tuning logic in
tune_cmd.cpp.
In `@tune_cmd.cpp`:
- Around line 758-782: Update the gradient path around score_from_components and
accumulate_gradient: skip accumulation when the component sentinel produces a
forced-zero score, and scale common_factor by the scorer’s rule-50 factor (100 -
rule50) / 100 before calling accumulate_gradient. Preserve the existing loss
calculation and other gradient behavior for non-sentinel positions.
---
Nitpick comments:
In `@eval.cpp`:
- Line 263: Remove the unused developedEg parameter from the TUNE registration
and any associated tuning declarations, since the developed-piece endgame term
is disabled and no longer affects evaluation or gradients.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fe80412-e773-466d-90f4-d1e697a769a8
📒 Files selected for processing (6)
.github/workflows/games.ymleval.cppeval.hspsa.pytune.cpptune_cmd.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- eval.h
- spsa.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/games.yml:
- Around line 141-146: Update the rounds calculation in the workflow’s shard
setup to avoid silently dropping remainder rounds: either distribute extra
rounds across the 30 shards so the total equals ROUNDS, or validate that ROUNDS
is a positive multiple of 30 and reject other values before calculating
shard_rounds.
- Around line 138-147: Merge the duplicate run blocks into one complete shell
script for the existing workflow step. Move ROUNDS from a step-level key into
env, retain its decimal validation and shard_rounds calculation, remove the
dangling continuation, and invoke tools/fastchess with the computed rounds
before the remaining flags.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f696928-5b9b-4de0-94d4-cbe9918fa1c9
📒 Files selected for processing (4)
.github/workflows/games.ymlWeights.heval.cpptune_cmd.cpp
💤 Files with no reviewable changes (1)
- Weights.h
🚧 Files skipped from review as they are similar to previous changes (2)
- tune_cmd.cpp
- eval.cpp
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent generated fixes only for
Lines 124–153 TC: ${{ github.event.inputs.tc }}
ELO0: ${{ github.event.inputs.elo0 }}
ELO1: ${{ github.event.inputs.elo1 }}
+ ROUNDS: ${{ github.event.inputs.rounds }}
run: |
set -eo pipefail
chmod +x new-engine/engine
chmod +x base-engine/$OUTPUT_EXEC
chmod +x tools/fastchess
chmod +x tools/ordo-linux64
- tools/fastchess \
- -engine cmd=new-engine/engine name=new \
- -engine cmd=base-engine/$OUTPUT_EXEC name=base \
- -openings file=tools/UHO_Lichess_4852_v1.epd format=epd order=random \
- -each tc=$TC -startup-ms 600000 -ucinewgame-ms 600000 -ping-ms 600000 \
- ROUNDS: ${{ github.event.inputs.rounds }}
- run: |
- set -eo pipefail
if [[ ! "$ROUNDS" =~ ^[0-9]+$ ]]; then
echo "rounds must be a decimal integer" >&2
exit 1
fi
rounds_value=$((10#$ROUNDS))
+ if [[ $((rounds_value % 30)) -ne 0 || $rounds_value -le 0 ]]; then
+ echo "rounds must be a positive multiple of 30" >&2
+ exit 1
+ fi
shard_rounds=$((rounds_value / 30))
+ tools/fastchess \
+ -engine cmd=new-engine/engine name=new \
+ -engine cmd=base-engine/$OUTPUT_EXEC name=base \
+ -openings file=tools/UHO_Lichess_4852_v1.epd format=epd order=random \
+ -each tc=$TC -startup-ms 600000 -ucinewgame-ms 600000 -ping-ms 600000 \
-rounds "$shard_rounds" -repeat -log level=err \
-concurrency $(nproc) -testEnv -sprt elo0=$ELO0 elo1=$ELO1 alpha=0.05 beta=0.05 \
- -pgnout notation=san nodes=true \
- file=games_${{ matrix.shard }}.pgn \
+ -pgnout notation=san nodes=true file=games_${{ matrix.shard }}.pgn \
| tee results_${{ matrix.shard }}.txt
- uses: actions/upload-artifact@v4 |
2 participants
not the stockfish as the branch name guaranteed
splitted piece middlegame and endgame values
Passed STC: https://github.com/winapiadmin/cppchess_engine/actions/runs/29553686707
Passed LTC: https://github.com/winapiadmin/cppchess_engine/actions/runs/30164194393