fix: sectioner does not consider separator length#1858
Merged
Conversation
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
from
October 24, 2023 17:00
3ec0d60 to
4491f36
Compare
scanny
force-pushed
the
scanny/chunk-by-title-is-rude
branch
from
October 24, 2023 17:01
72022ad to
a7ab984
Compare
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
from
October 24, 2023 17:42
4491f36 to
c6b558c
Compare
scanny
force-pushed
the
scanny/chunk-by-title-is-rude
branch
3 times, most recently
from
October 24, 2023 18:32
79e47bd to
11854fd
Compare
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
from
October 24, 2023 18:33
8198dc8 to
4d93ac5
Compare
scanny
marked this pull request as ready for review
October 24, 2023 19:24
scanny
force-pushed
the
scanny/chunk-by-title-is-rude
branch
from
October 24, 2023 21:22
11854fd to
6b8a3ce
Compare
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
2 times, most recently
from
October 24, 2023 22:39
7ed01c7 to
08bd213
Compare
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
from
October 25, 2023 00:40
08bd213 to
fa158e8
Compare
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
from
October 25, 2023 05:22
fa158e8 to
640314b
Compare
Coniferish
reviewed
Oct 26, 2023
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
2 times, most recently
from
October 26, 2023 19:37
72cfe88 to
20bb5a0
Compare
qued
reviewed
Oct 26, 2023
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
from
October 26, 2023 20:26
8c1141c to
400cac4
Compare
scanny
force-pushed
the
scanny/sectioner-considers-separator
branch
from
October 26, 2023 20:33
400cac4 to
405a2cd
Compare
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
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.
sectioner-does-not-consider-separator-length
Executive Summary. A primary responsibility of the sectioner is to minimize the number of chunks that need to be split mid-text. It does this by computing text-length of the section being formed and "finishing" the section when adding another element would extend its text beyond the window size.
When element-text is consolidated into a chunk, the text of each element is joined, separated by a "blank-line" (
"\n\n"). The sectioner does not currently consider the added length of separators (2-chars each) and so forms sections that need to be split mid-text when chunked.Chunk-splitting should only be necessary when the text of a single element is longer than the chunking window.
Example
Want:
Got:
Technical Summary
Because the sectioner does not consider separator (
"\n\n") length when it computes the space remaining in the section, it over-populates the section and when the chunker concatenates the element text (each separated by the separator) the text exceeds the window length and the chunk must be split mid-text, even though there was an even element boundary it could have been split on.Fix
Consider separator length in the space-remaining computation.
The solution here extracts both the
section.text_lengthandsection.space_remainingcomputations to a_TextSectionBuilderobject which removes the need for the sectioner (_split_elements_by_title_and_table()) to deal with primitives (List[Element], running text length, separator length, etc.) and allows it to focus on the rules of when to start a new section.This solution may seem like overkill at the moment and indeed it would be except it forms the foundation for adding section-level chunk combination (fix: dissociated title elements) in the next PR. The objects introduced here will gain several additional responsibilities in the next few chunking PRs in the pipeline and will earn their place.