Skip to content

Commit 81ea9c1

Browse files
derek73claude
andcommitted
Fix three defects in the new ambiguity emitters
All three are mine, from the previous round, found by review. 1. Only one SUFFIX_OR_FAMILY survived per segment. ambiguous_pick was a single slot the peel loop overwrote, so "John Smith MA JD" made two coin-flips and reported one -- defeating the point of reporting. Collect them instead. The sibling SUFFIX_OR_NICKNAME emitter already did this correctly, so it was an oversight, not a design. 2. The detail string was wrong under two of three name orders. The comment claimed the unpeeled piece "stays the last name piece -- the family name under every order"; _name_positions maps a two-piece name to [FAMILY, GIVEN] under FAMILY_FIRST, so it is the GIVEN name there and the text said otherwise. Ambiguity.detail is public, so this misinformed callers. Now reads the role back after assignment. 3. Origin resolution was quadratic, and so was the check it fed. Resolving each ambiguity's offset rescanned every token, and ParsedName.__post_init__'s subset check then did `tok not in self.tokens` per referenced token -- O(ambiguities x tokens) twice over. Reachable because the new closer sweep can emit one ambiguity per delimiter character. Bisect over the (already sorted) token starts, and hash the token tuple once: ') ' * N before after N=1600 182 ms 12 ms N=3200 -- 25 ms (linear) The subset check was pre-existing but only became reachable at scale once ambiguities started carrying tokens. Verified: differential harness 486 names, 2 intentional diffs, 0 unexplained. The review separately confirmed the 8147ac6 peel change against 304 generated shapes under all three name orders -- 86 changed, all 86 from disagreeing with v1 to matching it, 0 regressions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent cc7c6a8 commit 81ea9c1

4 files changed

Lines changed: 71 additions & 21 deletions

File tree

nameparser/_pipeline/_assign.py

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,12 @@ def _assign_main(seg_idx: int, state: ParseState,
124124
# every piece is a strict suffix (v1's are_suffixes tail rule, with
125125
# the roman-numeral special: a final roman numeral after a
126126
# non-initial piece is a suffix)
127-
# (piece, reading taken, reading declined) when the peel had to
128-
# resolve a bare ambiguous acronym; both directions are guesses
129-
ambiguous_pick: tuple[tuple[int, ...], str, str] | None = None
127+
# every bare ambiguous acronym the peel had to resolve -- one
128+
# coin-flip each, in either direction, so this collects rather than
129+
# overwrites. The role each ends up with is read back after
130+
# assignment, since which role "not peeled" means depends on
131+
# name_order.
132+
ambiguous_picks: list[tuple[int, ...]] = []
130133
k = len(rest)
131134
while k > 0:
132135
piece = pieces[rest[k - 1]]
@@ -151,15 +154,15 @@ def _assign_main(seg_idx: int, state: ParseState,
151154
bare_ambiguous = (len(piece) == 1
152155
and "vocab:suffix-ambiguous" in tokens[piece[0]].tags)
153156
if bare_ambiguous and k - 1 >= 2:
154-
ambiguous_pick = (piece, "a suffix", "an ordinary surname")
157+
ambiguous_picks.append(piece)
155158
k -= 1
156159
continue
157160
if bare_ambiguous and k >= 2:
158-
# not peeled, so it stays the last name piece -- the family
159-
# name under every order. (k < 2 means it is the only piece
160-
# left and lands in the given position, which is not the
161-
# fork this reports.)
162-
ambiguous_pick = (piece, "a family name", "a post-nominal")
161+
# not peeled, so it stays the last NAME piece -- which role
162+
# that is depends on name_order, so the detail reads it back
163+
# below. (k < 2 means it is the only piece left, which is
164+
# not the fork this reports.)
165+
ambiguous_picks.append(piece)
163166
break
164167
name_pieces, suffix_pieces = rest[:k], rest[k:]
165168
if not name_pieces and suffix_pieces:
@@ -170,13 +173,19 @@ def _assign_main(seg_idx: int, state: ParseState,
170173
_set_roles(tokens, pieces[piece_idx], roles[pos])
171174
for piece_idx in suffix_pieces:
172175
_set_roles(tokens, pieces[piece_idx], Role.SUFFIX)
173-
if ambiguous_pick is not None:
174-
piece, taken, declined = ambiguous_pick
176+
for piece in ambiguous_picks:
177+
token = tokens[piece[0]]
178+
# every pick was assigned a role just above; the fallback only
179+
# keeps the wording sane if a future path reports before then
180+
role = token.role.value if token.role is not None else "name"
181+
taken, declined = (
182+
("a suffix", "a name part") if token.role is Role.SUFFIX
183+
else (f"a {role} name", "a post-nominal"))
175184
ambiguities.append(PendingAmbiguity(
176185
AmbiguityKind.SUFFIX_OR_FAMILY,
177-
f"{tokens[piece[0]].text!r} written without periods is both "
178-
f"a post-nominal and a surname; read as {taken} rather than "
179-
f"{declined}",
186+
f"{token.text!r} written without periods is both a "
187+
f"post-nominal and an ordinary name; read as {taken} "
188+
f"rather than {declined}",
180189
tuple(piece)))
181190
# leading ambiguous particle read as a name (#121 surfaced)
182191
if name_pieces:

nameparser/_pipeline/_tokenize.py

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
"""
1919
from __future__ import annotations
2020

21+
import bisect
2122
import dataclasses
2223
import re
2324

@@ -97,11 +98,23 @@ def tokenize(state: ParseState) -> ParseState:
9798
# offset back out of its detail string. An offset inside a masked
9899
# region belongs to no token; those keep an empty tuple, which the
99100
# kind's contract already allows.
101+
# Bisect rather than rescan: the closer sweep can emit one
102+
# ambiguity per delimiter character, so a linear scan per ambiguity
103+
# is quadratic on pathological input (') ' * 1600 spent 180ms here,
104+
# against 9ms before the sweep existed). Spans are non-overlapping
105+
# and sorted just above, so the candidate is the last token whose
106+
# start is <= the offset.
107+
starts = [t.span.start for t in tokens]
108+
109+
def _containing(offset: int) -> tuple[int, ...]:
110+
i = bisect.bisect_right(starts, offset) - 1
111+
if i >= 0 and offset < tokens[i].span.end:
112+
return (i,)
113+
return ()
114+
100115
ambiguities = tuple(
101-
a if a.origin is None else dataclasses.replace(
102-
a, indices=tuple(
103-
i for i, t in enumerate(tokens)
104-
if t.span.start <= a.origin < t.span.end))
116+
a if a.origin is None
117+
else dataclasses.replace(a, indices=_containing(a.origin))
105118
for a in state.ambiguities)
106119
return dataclasses.replace(state, tokens=tuple(tokens),
107120
comma_offsets=tuple(sorted(commas)),

nameparser/_types.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -369,12 +369,18 @@ def __post_init__(self) -> None:
369369
f"offset {prev_end}"
370370
)
371371
prev_end = tok.span.end
372+
# Hash once rather than rescanning the tuple per referenced
373+
# token: a name can carry an ambiguity per token (a string of
374+
# stray delimiters does), and the linear form made construction
375+
# quadratic in their product. Set membership uses the same value
376+
# equality the tuple scan did -- Token is frozen and hashable.
377+
known = set(self.tokens) if self.ambiguities else ()
372378
for amb in self.ambiguities:
373379
for tok in amb.tokens:
374-
# `in` uses Token's value equality, not identity: this
375-
# only guarantees a value-equal token exists in
380+
# membership is by Token's value equality, not identity:
381+
# this only guarantees a value-equal token exists in
376382
# self.tokens, not that `tok` IS one of those objects.
377-
if tok not in self.tokens:
383+
if tok not in known:
378384
raise ValueError(
379385
f"Ambiguity token {tok.text!r} is not a subset of "
380386
f"this ParsedName's tokens"

tests/v2/test_parser.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,3 +211,25 @@ def test_delimited_ambiguous_acronym_reports_suffix_or_nickname() -> None:
211211
assert [t.text for t in n.ambiguities[0].tokens] == ["JD"]
212212
# the unambiguous one decided on vocabulary, so it is not a guess
213213
assert parse("Andrew Perkins (MBA)").ambiguities == ()
214+
215+
216+
def test_every_ambiguous_acronym_in_a_name_is_reported() -> None:
217+
# one coin-flip per acronym: a single-slot record dropped all but
218+
# the last, which defeats the point of reporting at all
219+
n = parse("John Smith MA JD")
220+
assert n.suffix == "MA, JD"
221+
assert [a.kind for a in n.ambiguities] == \
222+
[AmbiguityKind.SUFFIX_OR_FAMILY] * 2
223+
assert sorted(t.text for a in n.ambiguities for t in a.tokens) == \
224+
["JD", "MA"]
225+
226+
227+
def test_ambiguous_acronym_detail_names_the_role_it_got() -> None:
228+
# the unpeeled piece is the last NAME piece, which is the family
229+
# name only under GIVEN_FIRST -- FAMILY_FIRST puts it in given, so
230+
# the detail has to follow the role actually assigned
231+
fam_first = Parser(policy=Policy(name_order=FAMILY_FIRST))
232+
n = fam_first.parse("Jack MA")
233+
assert (n.family, n.given) == ("Jack", "MA")
234+
assert "given name" in n.ambiguities[0].detail
235+
assert "family name" not in n.ambiguities[0].detail

0 commit comments

Comments
 (0)