Repository navigation
fix(finqa_env): strip the year label before the percentage points fallback - #1569
Open
zuraiz-anjum wants to merge 1 commit into
Open
zuraiz-anjum wants to merge 1 commit into
zuraiz-anjum wants to merge 1 commit into
Conversation
…lback
When the ground truth has one \boxed{} per value, compute_reward strips a
label such as "2023: " from each predicted value before the first
comparison, but the fallback that reads "4.5%" as 4.5 percentage points
parsed the raw value again. parse_number("2023: 4.5%") fails, so a
labelled answer of percentage points scored 0 while the same answer
without labels scored 1.
Use the cleaned value in the fallback too.
This branch has not been deployed
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
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.
Summary
In the FinQA reward, when the ground truth has one
\boxed{}per value, the percentage points fallback parsed the raw predicted value instead of the one with its year label removed. So2023: 4.5%, 2024: 6.0%against\boxed{4.5} \boxed{6.0}scored 0, while4.5%, 6.0%scored 1. This passes the cleaned value to the fallback too.Type of Change
Alignment Checklist
Before submitting, verify:
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violated/pre-submit-pr(orbash .claude/hooks/lint.shand tests) and addressed all issuesRFC Status
Details
In
compute_reward(envs/finqa_env/server/rewards.py), the multi-value branch does this for each predicted value:"2023: 4.5%"becomes"4.5%"(pred_val_cleaned).normalize_answer(pred_val_cleaned)with the truth.4.5%reads as 0.045, so against 4.5 it fails.normalize_answer(pred_val, convert_percent=False)to read4.5%as 4.5 percentage points. This usedpred_val, so it parsed"2023: 4.5%".parse_numberstrips the%, triesfloat("2023: 4.5"), fails and returnsNone, and the reward is 0.The single-value path and the unlabelled multi-value path already get this right (
test_percentage_points_basic, and the newtest_percentage_points_multiple_boxed). The fix is one argument: usepred_val_cleanedin step 3.Test Plan
Two tests in
TestPercentagePointsNotationintests/envs/test_finqa_environment.py:test_percentage_points_multiple_boxed:4.5%, 6.0%against\boxed{4.5} \boxed{6.0}scores 1.0 and4.5%, 7.0%scores 0.0. This passes before and after, as a control.test_percentage_points_multiple_boxed_with_labels: the same answers with2023:and2024:labels. It fails on main and passes with the change.Run with
PYTHONPATH=src:envs pytest tests/envs/test_finqa_environment.py:test_percentage_points_multiple_boxed_with_labels), the other reward tests passed, the data-backed tests skipped.The three checks from
.claude/hooks/lint.sh(usort check,ruff format --check,ruff check) are clean on the files this PR touches; the findings a tree-wide run reports are in other files and are the same on main.Note
Low Risk
Localized scoring-path change in reward logic; improves parsing for labeled percentage answers with new tests, no security or broad behavioral surface.
Overview
Fix multi-valued FinQA reward for labeled percentage-point answers. In the multi-
\boxed{}branch ofcompute_reward, the percentage-points fallback now runsnormalize_answeron the label-strippedpred_val_cleanedinstead of the rawpred_val, so strings like2023: 4.5%, 2024: 6.0%correctly match\\boxed{4.5} \\boxed{6.0}.Adds regression tests for multiple
\\boxed{}values with comma-separated percentage answers, both with and without year labels, plus a mismatch case that should score 0.Reviewed by Cursor Bugbot for commit 4b5091e. Bugbot is set up for automated code reviews on this repo. Configure here.