Conversation
predict_dataset still has access to InputSample.spans before span_to_tag flattens them to per-token labels, so it now emits an annotation_span_id column: the index of the gold span covering each token, None for O. The column is only attached when at least one sample carries spans, keeping the 5-column contract for span-less datasets. SpanEvaluator prefers span ids over merge keys when reconstructing annotation spans (per sentence, falling back when the column carries no values). Ids identify entity instances rather than types, so two adjacent same-type entities stay separate at every level, including the same-type skip-word case merge keys could not distinguish. Prediction spans keep the existing reconstruction. _merge_keys_for now delegates column resolution to _merge_key_column instead of duplicating it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJ4tF86qwabSqafCuTMZFM
omri374
commented
Sep 7, 2026
| returns None when neither is available, in which case callers compare | ||
| the visible label alone. | ||
| """ | ||
| from presidio_evaluator.entity_mapping.data_objects import ( # noqa: PLC0415 |
Collaborator
Author
There was a problem hiding this comment.
can this import live on the top?
omri374
commented
Sep 7, 2026
| :return: DataFrame with exactly 5 columns: | ||
| :return: DataFrame with columns: | ||
| sentence_id, token, annotation, prediction, start_indices | ||
| [, annotation_span_id] |
Collaborator
Author
There was a problem hiding this comment.
this should be empty if not computed, not missing. The API should have a fixed set of columns
| sentence_id, token, annotation, prediction, start_indices | ||
| [, annotation_span_id] | ||
| """ | ||
| from presidio_evaluator.entity_mapping.data_objects import ( # noqa: PLC0415 |
Collaborator
Author
There was a problem hiding this comment.
Can this be on top?
| entity type matches the token's label wins, so a token's label and its | ||
| span id never disagree. | ||
| """ | ||
| spans = sample.spans or [] |
Collaborator
Author
There was a problem hiding this comment.
if we have spans, do we need to iterate tags?
| return df | ||
|
|
||
| @staticmethod | ||
| def _annotation_span_ids(sample: InputSample) -> list[int | None]: |
Collaborator
Author
There was a problem hiding this comment.
can the span id live in span to tag?
…hema Addresses review feedback on the draft PR: - span_to_tag optionally returns per-token span ids alongside the labels (return_span_ids=True). Ids index the caller's original span list and are threaded through overlap resolution, so a span split by a higher-score overlap keeps one identity. This replaces the post-hoc overlap-matching helper in BaseModel: ids now come from the same code that aligns spans to tokens, and iterating tags to stay consistent with that alignment is no longer needed. - InputSample stores the ids as span_ids when tags are created from spans; get_tags exposes them behind return_span_ids. - predict_dataset has a fixed 6-column schema: annotation_span_id is always present, all None when the sample's tags were provided directly instead of derived from spans. The evaluator already treats a value-less id column as absent per sentence, so merge keys still apply. - Imports of the shared column-name constants moved to module top in base_model and span_evaluator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJ4tF86qwabSqafCuTMZFM
…o claude/pr-191-review-hjozei
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
2 participants
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.
Builds on #191. Where #191 keeps collapsed labels distinguishable through merge keys (finest-grained label per token), this carries exact entity instance identity into the evaluator, so gold span boundaries are recovered rather than inferred from label runs.
Approach
span_to_tagis the code that aligns spans to tokens, so it is also the source of identity: withreturn_span_ids=Trueit returns, alongside the per-token labels, the index of the span that produced each label (None forO). Ids refer to the caller's original span order and are threaded through overlap resolution, so a span split by a higher-score overlap keeps one identity, and a token's label and id cannot disagree by construction.InputSamplestores the ids asspan_idswhen tags are created from spans (create_tags_from_span=True, the path all JSON-loaded datasets take), andget_tagsexposes them behind the same opt-in flag.BaseModel.predict_datasetcarries them into anannotation_span_idcolumn with a fixed 6-column schema: the column is always present, all None for samples whose tags were provided directly.SpanEvaluator._merge_key_columnprefers the span-id column over the merge-key column when it carries values for the sentence being processed, and_merge_keys_fordelegates to it instead of duplicating the resolution. Everything downstream (run-splitting in_create_spans, the identity check in_merge_adjacent_spans) is unchanged; ids flow through the same plumbing as merge keys.CanonicalMapperneeds no change: its level projections copy the DataFrame and only rewrite the label columns, so the id column passes through every level untouched.What this fixes beyond #191
Merge keys distinguish entity types, not instances, so two adjacent entities of the same type were still merged, the limitation #191's description calls out as out of scope:
"She visited Paris , London"— two LOCATION gold spans stayed one span at every level. With ids they stay two.Scope
Gold side only.
batch_predictreturns tags with no instance identity, so prediction spans keep the existing reconstruction (merge keys where present, visible labels otherwise). Aprediction_span_idconstant is defined and the evaluator resolves it symmetrically, so a source that knows its prediction boundaries can opt in later without evaluator changes.Fallback chain per sentence: span ids where present with values, else merge keys, else visible labels.
test_all_none_id_column_falls_back_to_merge_keyspins the middle step so an all-None id column cannot disable #191's fix.Verification
ruffcheck and format clean on changed files.test_span_to_tag.py(id/tag alignment, None for O, distinct ids for adjacent same-type spans, split span keeps one id, default return unchanged), 3 intest_base_model.py(id column values, fixed schema with all-None column, mixed datasets), 5 intest_span_evaluator.py(TestSpanIdColumn: touching same-type entities split, skip-word-separated same-type entities split, same-id fragments still merge, binary level, merge-key fallback), 1 intests/integration/test_data_objects.py(span_idspopulated bycreate_tags_from_span).predict_dataset's exact column list, updated for the new column.🤖 Generated with Claude Code
https://claude.ai/code/session_01LJ4tF86qwabSqafCuTMZFM