Skip to content

refactor: new event segmenter#66

Merged
kwaa merged 28 commits intomainfrom
refactor/event-segmentation
Apr 25, 2026
Merged

refactor: new event segmenter#66
kwaa merged 28 commits intomainfrom
refactor/event-segmentation

Conversation

@kwaa
Copy link
Copy Markdown
Member

@kwaa kwaa commented Apr 24, 2026

No description provided.

@kwaa kwaa requested a review from Lilia-Chen April 24, 2026 10:21
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request overhauls the event segmentation logic by implementing embedding-based boundary detection and LLM-assisted review. It introduces a locomo_segmenter example for debugging, adds confidence and score metadata to EventSegment, and includes math utilities for embedding analysis. Review feedback focuses on ensuring compatibility with stable Rust by avoiding unstable let-chains and recently stabilized methods like is_none_or, as well as optimizing vector normalization by removing redundant divisions.

Comment on lines +278 to +287
if index > 0
&& merge_indices.contains(&index)
&& let Some(previous) = merged.last_mut()
{
previous.events.extend(segment.events);
previous.boundary_after_confidence = segment.boundary_after_confidence;
previous.score = previous.score.min(segment.score);
} else {
merged.push(segment);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of 'let-chains' (&& let Some(...) = ...) is an unstable Rust feature (RFC 2497) and will cause compilation errors on the stable toolchain. This should be refactored into a nested if let block to ensure compatibility with stable Rust.

    if index > 0 && merge_indices.contains(&index) {
      if let Some(previous) = merged.last_mut() {
        previous.events.extend(segment.events);
        previous.boundary_after_confidence = segment.boundary_after_confidence;
        previous.score = previous.score.min(segment.score);
      }
    } else {
      merged.push(segment);
    }

.filter(|index| {
decisions
.get(index)
.is_none_or(|(label, confidence)| label.is_boundary() || *confidence < 0.55)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

is_none_or was stabilized in Rust 1.82.0. If the project targets an older toolchain or MSRV (Minimum Supported Rust Version), this will fail to compile. Using map_or(true, ...) is a more compatible alternative that achieves the same result.

Suggested change
.is_none_or(|(label, confidence)| label.is_boundary() || *confidence < 0.55)
.map_or(true, |(label, confidence)| label.is_boundary() || *confidence < 0.55)

Comment on lines +21 to +26
let count = (end - start) as f32;
let mut mean = prefix[end]
.iter()
.zip(&prefix[start])
.map(|(right, left)| (right - left) / count)
.collect::<Vec<_>>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The division by count is redundant because the resulting vector is immediately passed to normalize. Since normalization scales the vector to unit length based on its direction, the intermediate scaling factor 1/count does not affect the final unit vector. Removing this division avoids unnecessary floating-point operations.

Suggested change
let count = (end - start) as f32;
let mut mean = prefix[end]
.iter()
.zip(&prefix[start])
.map(|(right, left)| (right - left) / count)
.collect::<Vec<_>>();
let mut mean = prefix[end]
.iter()
.zip(&prefix[start])
.map(|(right, left)| right - left)
.collect::<Vec<_>>();

@kwaa kwaa merged commit 6111034 into main Apr 25, 2026
@kwaa kwaa deleted the refactor/event-segmentation branch April 25, 2026 16:15
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.

1 participant