Fix/152 faithfulness short claims not supported - #1015
Open
gzam1028 wants to merge 8 commits into
Open
Conversation
Selects issue ascherj#152 (faithfulness checker cannot mark short claims as supported) and documents the problem summary and setup confirmation.
…se support Adds a failing test demonstrating that FaithfulnessChecker._is_supported() requires >= 2 meaningful token overlaps regardless of claim length. A short claim like "Good communicator" has only 2 content tokens total, so any paraphrase (e.g. "excellent" instead of "good") drops the overlap to 1 and the claim is marked unsupported even though it is clearly true. Also adds missing type annotations across the file and completes two pre-existing tests that asserted nothing, so pre-commit hooks pass. This is a reproduction commit only — no fix yet.
Lays out the scaling-overlap-threshold approach for _is_supported(), ordered sub-tasks, and known risks (including the intentional flip of test_minimum_overlap_required's expected value).
Links the reproduction commit and PLAN.md, and summarizes the reproduction and open blockers going into Week 9.
FaithfulnessChecker._is_supported() required a flat >= 2 meaningful token overlap between a claim and its context, regardless of how many content words the claim actually had. Claims with only 1-2 meaningful tokens (e.g. "Good communicator") could never be marked supported once any one of those words was paraphrased, since the overlap could never reach 2. Lower the required overlap to 1 for claims with 2 or fewer meaningful tokens, while keeping the original floor of 2 for longer claims that have room for a paraphrase and still need real evidence to avoid a single lucky keyword match passing a vague claim. Updates test_minimum_overlap_required, which previously documented the buggy behavior on purpose (Python expertise vs. Python now correctly passes with a 2-token claim's 1-token floor). Adds edge case coverage for single-token claims and longer claims that should stay unsupported. make test-unit: 3 known pre-existing failures remain in this file, unrelated to this fix — test_partial_support_returns_middle_score and test_multiple_context_chunks fail due to a separate bug in _extract_claims() not splitting on commas/"and", and test_none_context_chunk_text fails due to issue ascherj#153 (crash on context chunk text: None). Confirmed via git stash comparison that these failures pre-date this change and are not introduced by it. Fixes ascherj#152
Links the submitted PR (ascherj#1015) and summarizes what was built, tests added, and self-review confirmation.
Documents that no reviewer feedback has come in on PR ascherj#1015 (expected per the Su26 note) and completes the five-part module reflection.
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
FaithfulnessChecker._is_supported()required a flat>= 2meaningful token overlap between a claim and its context, regardless of how many content words the claim actually had. Short claims (2 or fewer meaningful tokens, e.g. "Good communicator") could never be marked supported once one of those words was paraphrased in the context, since the overlap could never reach 2. This PR lowers the required overlap to 1 for claims with 2 or fewer meaningful tokens, while keeping the original floor of 2 for longer claims that still need real evidence.Issue
Closes #152
Changes
rag/evaluator/faithfulness_checker.py:_is_supported()now computes the claim's own meaningful token count and requires only 1 overlapping token when that count is 2 or fewer, instead of a flat 2.tests/unit/test_faithfulness_checker.py:test_short_claim_with_partial_paraphrase_should_be_supported(the reproduction test for Faithfulness checker can never mark short claims as supported #152 — now passes).test_minimum_overlap_required's expected value ("Python expertise" vs. "Python" now correctly passes under the new 1-token floor for short claims — it previously assertedFalseon purpose to document the bug).test_long_claim_with_single_overlap_stays_unsupported,test_single_meaningful_token_claim_matches,test_single_meaningful_token_claim_no_matchfor edge case coverage.Testing
make test-unit) — see note below on pre-existing failuresmake test-integration) — not applicable, no integration surface for this changemake lint) — for the files changed in this PR (ruff checkon both files passes clean; the full-repomake lintfails on 180 pre-existing, unrelated errors — see note)make typecheck) — for the files changed in this PRNote on pre-existing failures: This repo has known pre-existing issues unrelated to #152. I confirmed via
git stashcomparison that none of the following are introduced by this PR:test_partial_support_returns_middle_scoreandtest_multiple_context_chunksfail due to a separate bug in_extract_claims()not splitting claims on commas/"and".test_none_context_chunk_textfails due to issue Faithfulness checker crashes when a context chunk hastext: None#153 (crash when a context chunk'stextisNone).make lint/make checkfail repo-wide due to 180 pre-existing ruff errors across unrelated files (181 without this PR's changes — this PR fixes one pre-existing import-order issue in the file it touches and introduces zero new lint errors).Screenshots / Demo
N/A — backend logic change, no UI surface.
Notes for Reviewers
The threshold change is intentionally conservative: it only lowers the bar for short claims (≤2 meaningful tokens) and leaves the original
>= 2requirement in place for everything else, to avoid making longer/vaguer claims too easy to pass on a single lucky keyword match. SeePLAN.mdon this branch for the full reasoning and the risks I considered (particularly the intentional flip oftest_minimum_overlap_required's expected value).