fix(scoring): DROP recognises numeric tokens before stripping punctuation, like the official metric - #424
Merged
James-CUDA merged 2 commits intoJul 23, 2026
Conversation
… can validate this PR main currently fails Ruff/pytest by itself: src/trinity/analysis/__init__.py carries unresolved merge-conflict markers (James-CUDA#404), so every PR's merge-ref CI is red before it reaches the PR's own changes. This commit applies the only correct resolution — keep all three import blocks (union_diagnostics, oracle_column, definition_of_done), the same resolution James-CUDA#402/James-CUDA#405 propose — purely to unblock CI validation of the scoring fix in this branch. Once the dedicated fix lands on main this resolves to a no-op in the merge (or will be dropped by rebase); credit for the James-CUDA#404 fix belongs to those PRs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nctuation
_normalize_token stripped edge punctuation — a set that includes "." —
BEFORE trying the numeric parse, so a leading-decimal token like ".5"
lost its point and normalized to "5.0": equal to a gold "5" (a wrong
answer, ten times off, graded correct) and unequal to the
value-identical gold "0.5" (a correct answer graded wrong). The
official DROP _remove_punc tests _is_number first and leaves numeric
tokens untouched, which the module docstring already promises to match.
Fix: try the numeric parse on the raw token (commas removed) first;
only non-numeric tokens go through edge-punctuation stripping. All
surrounding behaviors are unchanged — currency ("$16"), sentence-final
periods ("16."), thousands commas ("1,234.5"), preserved signs ("-5"),
and word tokens normalize exactly as before.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type
General improvement
What does this PR do?
Fixes #423.
_normalize_tokenstrips edge punctuation — a set that includes.— before trying the numeric parse, so a leading-decimal token like.5loses its point and normalizes to"5.0":Answer: .50.5Answer: .55Root cause: the official DROP
_remove_punc(allennlpdrop_em_and_f1) tests_is_numberfirst and leaves numeric tokens untouched; this module's docstring promises to match that normalization but ran the strip first, corrupting exactly the token shape the official order protects. Both a false positive (an answer ten times off graded correct) and a false negative (a correct shorthand answer graded wrong).Fix approach (minimal): try the numeric parse on the raw token (commas removed) first; only non-numeric tokens fall through to edge-punctuation stripping. One code path added, nothing removed — currency (
$16), sentence-final periods (16.), thousands commas (1,234.5), preserved signs (-5), and word tokens normalize byte-identically to before (pinned by tests).Why is it needed?
DROP golds include decimal averages/rates (
0.5,24.5) and.5-style shorthand is ordinary model output, so this diverges from the official metric on real grading traffic. Same bug class as the merged #395 (hyphen tokenization "like the official metric"). No existing test covered a leading-decimal token.Tests:
tests/test_drop_leading_decimal.py(7 cases) pins the number-first order (.5/-.5), both grading directions throughscore_drop, and regression-guards currency/edge-punctuation/sign/comma/word behavior unchanged.Note on CI: current
mainfails Ruff/pytest by itself —src/trinity/analysis/__init__.pycarries unresolved merge-conflict markers (#404, fixed by #402/#405). Like #420, this branch carries onechore(ci)commit applying the only correct resolution (credit for #404 belongs to those PRs) purely so CI can validate this fix; it resolves to a no-op once the dedicated fix lands, and I will rebase it away on request. With it, the full suite passes locally:1736 passed, 2 skipped(+7 new),ruff checkclean.Checklist
pytest tests/(1736 passed, 2 skipped locally; 3 Windows-only console-encoding failures unrelated to this change pass on Linux)ruff check src/mypy src/(for changed files)🤖 Generated with Claude Code