Skip to content

FIX Keep original_float_value in line with the verdict for inverted and combined scores - #2895

Open
Utkarsh Bahuguna (u7k4rs6) wants to merge 14 commits into
microsoft:mainfrom
u7k4rs6:fix/true-false-wrapper-original-float
Open

Utkarsh Bahuguna (u7k4rs6) wants to merge 14 commits into
microsoft:mainfrom
u7k4rs6:fix/true-false-wrapper-original-float

Conversation

@u7k4rs6

@u7k4rs6 Utkarsh Bahuguna (u7k4rs6) commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2894.

TrueFalseInverterScorer now drops original_float_value from the inverted score, since the wrapped threshold float describes the uninverted verdict. The true/false aggregators drop it when they combine more than one score, and keep it for a single score, where it still matches. normalize_score_to_float then falls back to the verdict for inverted and combined scores.

Tests use real PlagiarismScorer + FloatScaleThresholdScorer scores with no mocks: the inverted score normalizing to its verdict with no leaked float, a two-scorer AND composite normalizing to 0.0, and a single-scorer composite keeping the float. The first two fail on main. tests/unit/score: 2832 passed.

@romanlutz

Copy link
Copy Markdown
Contributor

I responded on the issue.

@u7k4rs6

Copy link
Copy Markdown
Contributor Author

Went with that in 24fb820. The inverter now drops the child float instead of flipping it, so inverted and multi-score results both normalize from their own verdict. Direct threshold scores and single-scorer composites keep it.

@romanlutz

Copy link
Copy Markdown
Contributor

This makes sense to me but I want Richard Lundeen (@richlundeen) to take a look. AFAIK this won't break anything.

@u7k4rs6

Copy link
Copy Markdown
Contributor Author

Makes sense to land #2916 first. I tried merging it into this branch locally and it goes in cleanly. The only thing that changes is the assertion in test_threshold_and_inverter_preserve_each_judgment_async that expects original_float_value on the inverted result, which this PR would flip to not present. With #2916 the threshold's own float stays on its intermediate score, so dropping it from the wrapper's copy doesn't lose anything. I'll merge main here and update that test once #2916 is in.

@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 1, 2026
@u7k4rs6

Copy link
Copy Markdown
Contributor Author

Merged main now that #2916 is in, and updated test_threshold_and_inverter_preserve_each_judgment_async in 7653834. The inverted result no longer has original_float_value, and the retained threshold score still has 0.8. tests/unit/score and tests/unit/executor pass.

@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 1, 2026
@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 1, 2026
@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 1, 2026
@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 1, 2026
@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 2, 2026
@u7k4rs6

Copy link
Copy Markdown
Contributor Author

The merge queue failures were my test fixture still calling add_message_to_memory, which main now deprecates and the suite turns into an error. It uses add_message_to_memory_async now (0cb7b7e), and I merged main again. tests/unit/score and tests/unit/executor pass locally. Auto-merge should be able to pick it up once CI runs.

auto-merge was automatically disabled October 2, 2026 11:34

Head branch was pushed to by a user without write access

@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Oct 2, 2026
@u7k4rs6

Copy link
Copy Markdown
Contributor Author

Richard Lundeen (@richlundeen) CI is green on 0cb7b7e now, but my push turned auto-merge off. Could you re-enable it when you get a chance?

@u7k4rs6

Copy link
Copy Markdown
Contributor Author

The one failure here is unrelated: tests/unit/scenario/core/test_scenario_partial_results.py::test_run_async_cancellation_persists_progress_cleans_workers_and_resumes timed out on macOS 3.13 at its 5s wait_for (line 506), and the rest of the matrix got cancelled after it. This PR only touches the score wrappers, the test passes locally (5 of 5), and main's latest build_and_test also has an async timeout flake (c2ae5ee). The branch is already up to date with main. Could someone re-run the failed jobs? I don't have rights to.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG Inverted and combined true/false scores keep a float that contradicts their verdict

3 participants