fix: calculate float context size per chunk in overlap refinery#558
fix: calculate float context size per chunk in overlap refinery#558anaslimem wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes how OverlapRefinery computes float-based overlap sizes so that each chunk’s overlap is proportional to the receiving chunk’s token_count (instead of the chunk providing the context).
Changes:
- Update
_refine_prefix/_refine_suffixto compute floateffective_context_sizefrom the receiving chunk’stoken_count. - Adjust the reuse/correctness test data/comments for float
context_size.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/chonkie/refinery/overlap.py |
Computes float overlap size per receiving chunk in prefix/suffix refinement loops. |
tests/refinery/test_overlap_refinery.py |
Updates the float-context reuse test setup and assertions (currently too weak to catch regressions). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # This ensures each chunk gets overlap proportional to its own size | ||
| if isinstance(self.context_size, float): | ||
| effective_context_size = int(self.context_size * prev_chunk.token_count) | ||
| effective_context_size = int(self.context_size * chunk.token_count) |
There was a problem hiding this comment.
When context_size is a float, int(self.context_size * chunk.token_count) can become 0. Even though suffix token overlap would yield an empty slice, recursive mode uses effective_context_size as a splitting step, which would raise at runtime if it becomes 0. Consider guarding/clamping the computed effective_context_size before using it.
| effective_context_size = int(self.context_size * chunk.token_count) | |
| effective_context_size = int(self.context_size * chunk.token_count) | |
| if self.mode == "recursive": | |
| effective_context_size = max(1, effective_context_size) |
| effective_context_size = int(self.context_size * chunk.token_count) | ||
|
|
||
| # Calculate the overlap context | ||
| # Get context from the previous chunk |
There was a problem hiding this comment.
The comment says "Get context from the previous chunk", but in suffix mode prev_chunk = chunks[i + 1] is the next chunk (the provider of the suffix context). Updating the comment will prevent confusion when reading/debugging this loop.
| # Get context from the previous chunk | |
| # Get suffix context from the next chunk |
| # Each chunk gets context proportional to its own size | ||
| assert small_context is not None and small_context != "" | ||
| assert large_context is not None and large_context != "" |
There was a problem hiding this comment.
This test no longer asserts the behavior that the PR claims to fix. With the pre-fix code, both small_context and large_context would still be non-empty, so these assertions would pass. To make the test catch regressions, assert the context length/content matches int(context_size * receiving_chunk.token_count) (and/or differs from the provider chunk’s proportional size), e.g. for the default character tokenizer len(refined_*[0].context) should equal the expected effective context size.
| # Get context from the previous chunk | ||
| context = self._get_prefix_overlap_context(prev_chunk, effective_context_size) |
There was a problem hiding this comment.
When context_size is a float, int(self.context_size * chunk.token_count) can evaluate to 0 for small chunks. That can produce incorrect overlap (e.g., in token+prefix mode, slicing with tokens[-0:] returns the entire chunk) and can also break recursive overlap where effective_context_size is used as a split step. Consider explicitly handling the 0 case (return empty context) or clamping to a minimum of 1 token/char before calling the overlap helpers.
| # Get context from the previous chunk | |
| context = self._get_prefix_overlap_context(prev_chunk, effective_context_size) | |
| # A fractional context size can truncate to 0 for very small chunks. | |
| # Treat that as "no overlap" instead of forwarding 0 into the overlap | |
| # helpers, where it can produce incorrect slicing behavior. | |
| if effective_context_size <= 0: | |
| context = "" | |
| else: | |
| # Get context from the previous chunk | |
| context = self._get_prefix_overlap_context( | |
| prev_chunk, effective_context_size | |
| ) |
There was a problem hiding this comment.
Code Review
This pull request updates the OverlapRefinery to calculate overlap context size based on the token count of the chunk receiving the context, rather than the chunk providing it. This ensures that the overlap is proportional to the size of the chunk being refined. The changes include updates to both prefix and suffix refinement methods and corresponding test adjustments. A review comment identified a misleading comment in the suffix refinement logic that should be corrected to refer to the 'next' chunk instead of the 'previous' one.
| effective_context_size = int(self.context_size * chunk.token_count) | ||
|
|
||
| # Calculate the overlap context | ||
| # Get context from the previous chunk |
There was a problem hiding this comment.
The comment incorrectly states that context is retrieved from the "previous" chunk. In the suffix refinement method, context is actually retrieved from the following chunk (at index i + 1) to be appended to the current chunk's suffix.
| # Get context from the previous chunk | |
| # Get context from the next chunk |
Summary
Fix the float context size calculation in
OverlapRefineryto use each chunk's own token count instead of the chunk providing the context.Solution
Calculate context size based on the chunk receiving the context:
This ensures each chunk gets overlap proportional to its own size.
Files Changed