Skip to content

Fix validate_frequency_capital_words treating "around" as exact equality#1645

Open
Chessing234 wants to merge 1 commit into
allenai:mainfrom
Chessing234:fix/validate-frequency-capital-words-around-tolerance
Open

Fix validate_frequency_capital_words treating "around" as exact equality#1645
Chessing234 wants to merge 1 commit into
allenai:mainfrom
Chessing234:fix/validate-frequency-capital-words-around-tolerance

Conversation

@Chessing234
Copy link
Copy Markdown
Contributor

Bug

validate_frequency_capital_words in open_instruct/if_functions.py treats the \"around\" quantifier as exact equality:

elif quantifier == "around":
    return len(words) == N   # strict equality — wrong

This means a response with N±1 all-capital words always fails the constraint, contrary to the IFEval spec's intent for "approximately N".

Root cause

Every other constraint function in the same file applies a tolerance band for \"around\":

  • validate_word_constraintabs(actual_count - N) <= max(round(N * 0.1), 1) (±10 %, min 1)
  • verify_sentence_constraintabs(actual_count - N) <= 1

validate_frequency_capital_words was left with strict equality, making \"around\" indistinguishable from \"exactly\".

Fix

Apply the same ±10 % (minimum 1) tolerance used by validate_word_constraint, since both functions count word-level occurrences.

The \"around\" branch returns `len(words) == N`, making it behave
identically to an exact match rather than the intended approximate
tolerance. Every analogous constraint function in the same file uses a
tolerance band: `validate_word_constraint` accepts ±10 % (or ±1 word),
`verify_sentence_constraint` accepts ±1 sentence. Only
`validate_frequency_capital_words` was left with strict equality,
so a response with N±1 all-capital words would always fail \"around\".

Use the same 10 % (minimum 1) tolerance applied by
`validate_word_constraint` for consistency.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the validate_frequency_capital_words function to implement a tolerance range for the 'around' quantifier, replacing the exact match requirement. The feedback suggests extracting this tolerance logic into a shared helper function to improve maintainability and reduce code duplication across the codebase.

return len(words) >= N
elif quantifier == "around":
return len(words) == N
return abs(len(words) - N) <= max(round(N * 0.1), 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic for the 'around' quantifier in validate_frequency_capital_words is now consistent with validate_word_constraint. However, for better maintainability and to avoid code duplication, consider extracting the tolerance calculation into a shared helper function, as this logic is now used in multiple places.

Copy link
Copy Markdown
Collaborator

@finbarrtimbers finbarrtimbers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that would have caught this?

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.

2 participants