fix(fusion): bound moment components to shared overlapping interval (#87) - #141
Conversation
tulayha
left a comment
There was a problem hiding this comment.
Thanks for working on #87. Using a shared-overlap boundary is a reasonable direction for the initial bounded rule, and the implementation continues to preserve the original hits for inspection.
There is one remaining behavior to address before merge: with the regression example in this PR, the returned moment ranges are still broad and overlap each other. I have left the specific case inline.
For scope clarity, you do not need to implement the broader #76 comparison or the clip-level work from #83 in this PR. Those remain separate pieces of work. Once the returned bounds and regression coverage are corrected, this PR can close #87.
|
|
||
| self.assertEqual(len(result.moments), 2) | ||
| moment_1, moment_2 = result.moments | ||
| self.assertEqual(moment_1.start, 10.0) |
There was a problem hiding this comment.
This now separates scene:c into a second component, but the first returned moment is still 10–25: it contains the bridging 11–25 hit, and fusion calculates the moment bounds using the minimum start and maximum end. The second moment is 24–26, so the two returned moments overlap and the first clip can still include the second appearance.
Could we adjust how the fused bounds are produced so the returned moments remain useful and separate, then assert both moments complete ranges here? Using the shared overlap window is one possible approach, while still retaining the original hit ranges for inspection.
| current: list[SearchHit] = [] | ||
| current_media: str | None = None | ||
| current_end = 0.0 | ||
| current_overlap_end = 0.0 |
There was a problem hiding this comment.
Small naming follow-up: this function no longer computes graph-connected components; it partitions hits using a shared-overlap boundary.
Could we rename _connected_components to something like _shared_overlap_components or _bounded_components while changing its behavior? This is not a blocker, but it will make the difference from the old transitive rule clearer for future contributors.
|
One small coordination request: could you also leave a comment on #87 confirming that you are working on it? Once you do, I can assign the issue to you. |
Related issue
Closes #87
Summary
When grouping search hits into moments in
_connected_components, VidXP previously combined hits whenever their time ranges touched or overlapped (hit.start <= current_end, withcurrent_end = max(current_end, hit.end)). This transitive interval chaining caused a single bridging hit spanning two otherwise separate moments (e.g. A overlaps B and B overlaps C, but A and C are disjoint) to collapse into one broad moment.This change bounds each moment component to hits that share a common overlapping interval (
current_overlap_end = min(current_overlap_end, hit.end)). Nearby duplicate hits covering the same scene or action still combine, while disjoint moments bridged by a longer hit remain separate occurrences.Validation
pytest tests/test_search_fusion.py -p no:asyncio(5 passed in 0.27s, including regression testtest_bridging_hit_does_not_merge_separate_momentsandtest_nearby_duplicate_hits_combine_into_one_moment)pytest tests/test_query_service.py tests/test_evidence_delivery.py -p no:asyncio(23 passed in 1.23s)ruff check src/vidxp/search_fusion.py tests/test_search_fusion.py(All checks passed)ruff format --check src/vidxp/search_fusion.py tests/test_search_fusion.py(2 files already formatted)