Skip to content

fix(executor): snap entity spans to word boundaries after regex match#29

Open
AZERDSQ131 wants to merge 1 commit into
KRLabsOrg:mainfrom
AZERDSQ131:fix/snap-entity-spans-to-word-boundaries
Open

fix(executor): snap entity spans to word boundaries after regex match#29
AZERDSQ131 wants to merge 1 commit into
KRLabsOrg:mainfrom
AZERDSQ131:fix/snap-entity-spans-to-word-boundaries

Conversation

@AZERDSQ131

Copy link
Copy Markdown

Closes #7

Implements option (b) from the issue: after substitute_template produces a span's text/start/end, trim leading/trailing whitespace and punctuation and adjust the offsets to match. This turns over-captures like "London," into "London" with correct offsets, so they line up with gold spans under the default text matching mode in evaluation.py::_match_entities instead of counting as both a false positive and a false negative.

Only edge characters are trimmed (whitespace + string.punctuation) — interior characters and the capture group the rule intended are never touched, per the issue's gotcha.

  • tests/test_executor.py: 5 new TestSubstituteTemplate cases (trailing punctuation, leading+trailing whitespace, wrapping quotes, no-op when already clean, all-punctuation text left unmodified to avoid producing an empty span) + 1 TestExecuteRegexRule acceptance test matching the issue's example (a rule that over-captures a trailing comma on "London,", asserting the produced span is "London" with offsets that round-trip through the source text).
  • Full suite: 317 passed, 1 skipped (pre-existing skip, unrelated).
  • ruff check, ruff format --check, mypy clean on the changed files.

@adaamko

adaamko commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thanks — the implementation is clean and the offset arithmetic is correct, but I don't think we can merge with string.punctuation as the trim set. It includes %, $, . and /, so correct captures get corrupted: a QUANTITY rule that rightly captures '50%' would be snapped to '50', MONEY '$5,000' to '5,000', 'no. 44774/98' style case references and abbreviations like 'Inc.' also get clipped. Under text matching those all flip from correct to wrong, so the fix for boundary slips would introduce a new class of them.

Suggestion: trim only whitespace plus sentence/wrapper punctuation — something like ,;:!?"'()[]{}<> — and leave $ % . # / & - _ + @ alone ('.' is the debatable one: trimming it fixes 'London.' but breaks 'Inc.'; I'd leave it in the span and let feedback repair handle sentence-final periods). Could you narrow the set, add '50%' / '$5,000' regression tests, and also tick the rights checkbox in the PR body? Happy to merge after that.

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.

NER span-boundary errors hurt rule recall

2 participants