Add method to find overlapping excerpts by rlskoeser · Pull Request #272 · Princeton-CDH/corppa

rlskoeser · 2026-04-16T20:55:05Z

Associated Issue(s): resolves #251

Changes in this PR

Moved logic for merging grouped excerpts into a method with the hope of making it reusable
Added new method for identifying overlapping excerpts; adapted from existing span overlap factor logic

Notes

Overlap length / factor logic is adapted from @laurejt span evaluation code
Would appreciate input what fields to return and field names (I don't love the _right default but haven't come up with anything better); extra fields have been helpful when testing, should they be optional? removed?

Review checklist

check logic for identifying high-confidence overlap
weigh in on returned field question above
check unit tests - are there important cases I should add? (more than one pair?)

codecov · 2026-04-16T20:56:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.82%. Comparing base (074661f) to head (5f9e59d).
⚠️ Report is 15 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #272      +/-   ##
===========================================
+ Coverage    79.76%   79.82%   +0.05%     
===========================================
  Files           23       23              
  Lines         2120     2126       +6     
===========================================
+ Hits          1691     1697       +6     
  Misses         429      429

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

laurejt

I've added my initial thoughts on merge_excerpts.py, but haven't looked at its unit tests yet.

Some higher-level musings:

Perhaps it'd be useful to add an additional field with excerpt length given how often we operate on it.
Would it ever be useful to use the map_groups method? I know it's generally slower, but perhaps it could be useful at some point...

Co-authored-by: Laure Thompson <602628+laurejt@users.noreply.github.com>

Assisted-by: Claude:sonnet-4-6 [ClaudeCode]

rlskoeser · 2026-04-21T14:52:45Z

@laurejt I've cleaned up based on your feedback and added the core objects to our sphinx docs (which I also split out into separate files for each top-level module to be more manageable). There are more changes in the PR now because I've cleaned up the docs, so please focus on the identify_overlapping_excerpts method: do you see any problems with the logic? Do you have an opinion on the fields returned? Are there any important cases you think I should cover in the unit tests?

laurejt

This is looking really good. I think it'd be nice to include poem id fields for identify_overlapping_excerpts's output. See my comments for some additional unit tests that you should consider adding primarily around the ordering logic of results.

laurejt · 2026-04-21T15:27:14Z

+    groups of excerpts into merged excerpts.  Fields are expected to correspond to
+    labeled excerpts (:class:`~corppa.poetry_detection.core.LabeledExcerpt`), and the


Is it possible for there to be a group of unlabeled excerpts that should be merged (i.e., the collective poem_id would be null)?

Possible but not very likely; I don't think any of the logic here would cause problems in that case. If you want to improve the language please suggest changes.

I think it's fine, just figured I'd ask. It's just a slight type misalignment since a LabeledExcerpt's poem_id field cannot be None

right, technically it could be a mix of labeled and unlabeled excerpts

laurejt · 2026-04-21T15:29:01Z

-            .explode()
-            .unique(),  # combine in a single list, no repeats
+            # NOTE: does not handle overlapping spans;
+            # currently assumes text spans match or first in group is complete


Does "complete" here mean is labeled?

I think what I meant is that it is the full text / longest span of the group but I agree it doesn't make a lot of sense - right now we're only using this on equal spans and I do have a note about that

laurejt · 2026-04-21T15:44:50Z

+        )
+    )
+
+    # what fields are needed here?


Including poem ids (when available) could also be useful

laurejt · 2026-04-21T16:00:03Z

    assert merged_ex1.identification_methods == {"manual", "refmatcha"}
+
+
+### test for identify_overlapping_excerpts


I think it's worth adding a couple unit tests for testing the behavior of specifying min_overlap_factor and min_overlap_chars especially the boundaries (i.e., greater than vs greater)

laurejt · 2026-04-21T16:11:56Z

@rlskoeser When I build the docs locally, I receive the following warning:

/Users/lauret/cdh-dev/ppa/corppa/README.md:36: WARNING: 'myst' cross-reference target not found: 'LICENSE' [myst.xref_missing]

Co-authored-by: Laure Thompson <602628+laurejt@users.noreply.github.com>

Resolves warning about unresolved link in readme

rlskoeser · 2026-04-21T17:32:55Z

Added missing target for license in the docs, added additional test cases.

rlskoeser added 2 commits April 16, 2026 11:51

Method to find overlapping excerpts; reusable logic for merging groups

5833bc4

Add unit tests for method for identify overlapping excerpts

317ca92

rlskoeser requested a review from laurejt April 16, 2026 20:59

laurejt reviewed Apr 20, 2026

View reviewed changes

Apply suggestions from code review

a617b0b

Co-authored-by: Laure Thompson <602628+laurejt@users.noreply.github.com>

rlskoeser force-pushed the feature/find-overlapping-excerpts branch from 9552772 to ed08cc7 Compare April 20, 2026 19:52

Simplify logic for filtering overlapping excerpt pairs

4b83c42

rlskoeser force-pushed the feature/find-overlapping-excerpts branch from ed08cc7 to 4b83c42 Compare April 20, 2026 20:09

rlskoeser added 5 commits April 21, 2026 10:10

Clean up & clarify comments for merge, remove unneeded code & sort

4b519b1

Split code docs out into one file per top-level module

8592b93

Assisted-by: Claude:sonnet-4-6 [ClaudeCode]

Add core objects to docs & document fields

bbf5e38

Improve docs for merge excerpts code

d23bedc

Update years in docs copyright statement

6339d5b

rlskoeser force-pushed the feature/find-overlapping-excerpts branch from 7763df1 to 6339d5b Compare April 21, 2026 14:45

Fix formatting in docstring

99ca067

rlskoeser requested a review from laurejt April 21, 2026 14:52

laurejt approved these changes Apr 21, 2026

View reviewed changes

rlskoeser and others added 3 commits April 21, 2026 13:20

Apply suggestions from code review

757935b

Co-authored-by: Laure Thompson <602628+laurejt@users.noreply.github.com>

Add license document to sphinx docs

33be42e

Resolves warning about unresolved link in readme

Clean up & additional test cases per @laurejt review

5f9e59d

rlskoeser merged commit a0d336c into develop Apr 21, 2026
9 checks passed

rlskoeser deleted the feature/find-overlapping-excerpts branch April 21, 2026 17:33

		groups of excerpts into merged excerpts. Fields are expected to correspond to
		labeled excerpts (:class:`~corppa.poetry_detection.core.LabeledExcerpt`), and the

		assert merged_ex1.identification_methods == {"manual", "refmatcha"}


		### test for identify_overlapping_excerpts

Conversation

rlskoeser commented Apr 16, 2026 • edited by laurejt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes in this PR

Notes

Review checklist

Uh oh!

codecov Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

laurejt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rlskoeser commented Apr 21, 2026

Uh oh!

laurejt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laurejt commented Apr 21, 2026

Uh oh!

rlskoeser commented Apr 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rlskoeser commented Apr 16, 2026 •

edited by laurejt

Loading

codecov Bot commented Apr 16, 2026 •

edited

Loading