fix: Fix split_start_idx and _split_overlap information in DocumentSplitter

[sjrl]

Related Issues

• fixes #issue-number

Proposed Changes:

The DocumentSplitter was incorrectly calculating the split_start_idx and _split_overlap information due to slight miscalculations of appropriate indices.

This was missed because the values of split_start_idx and _split_overlap weren't actually tested for in the unit tests.

How did you test it?

Expanded the unit tests to test the values of the incorrect keys.

Notes for the reviewer

Checklist

• I have read the contributors guidelines and the code of conduct
• I have updated the related issue with new insights and changes
• I added unit tests and updated the docstrings
• I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
• I documented my code
• I ran pre-commit hooks and fixed any issue

[coveralls]

Pull Request Test Coverage Report for Build 10075792294

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.003%) to 90.139%

---

Files with Coverage Reduction	New Missed Lines	%
components/retrievers/sentence_window_retrieval.py	3	83.33%

Totals
Change from base Build 10075624435:	-0.003%
Covered Lines:	6856
Relevant Lines:	7606

---

💛 - Coveralls

[vblagoje]

There seem to be some minor test failures @sjrl

[sjrl]

@vblagoje it should be fixed now. Issue was SentenceWindowRetriever had the same issue as the DocumentSplitter. Should be fixed now.

[davidsbatista]

@sjrl thanks for detecting and fixing this

[vblagoje]

@sjrl not directly related to your change but why do we set instance variable split_at from run invocation? A big smell IYAM

Can we avoid this bad practice in any way in this class?

[davidsbatista]

I have no clue, when I updated to contain the split_id and split_overlap it was already like that - what's the issue exactly?

[vblagoje]

I have no clue, when I updated to contain the split_id and split_overlap it was already like that - what's the issue exactly?

It's a bad practice basically. And we should see how we can avoid it. If not here than in separate PR

[davidsbatista]

And we should see how we can avoid it

I think it's enough to move the first block of _split_into_units into the _init_() and update the rest of the code accordingly, but I think that's out-of-scope for this PR.

[sjrl]

Hey @vblagoje I agree with @davidsbatista let's make that fix in a separate PR

[vblagoje]

Approved with a note we all agreed on.