Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added RecursiveCharacterSplitter #14

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

LexiestLeszek
Copy link
Contributor

Found the description on LangChain website and created my implementation, seems to be working. It will split documents recursively by different characters - starting with "\n\n", then "\n", then " ". This is nice because it will try to keep all the semantically relevant content in the same place for as long as possible.

Orig from langchain: https://js.langchain.com/docs/modules/indexes/text_splitters/examples/recursive_character

Found the description on LangChain website and created my implementation, seems to be working. It will split documents recursively by different characters - starting with "\n\n", then "\n", then " ". This is nice because it will try to keep all the semantically relevant content in the same place for as long as possible.

Orig from langchain: https://js.langchain.com/docs/modules/indexes/text_splitters/examples/recursive_character
@ZachNagengast ZachNagengast self-requested a review July 4, 2023 19:04
@LexiestLeszek
Copy link
Contributor Author

Hi! Let me know if I need to do anything for this PR to be submitted

@ZachNagengast
Copy link
Owner

Hi @LexiestLeszek looks like there was one test failure, but it may resolve itself with re-run. Doing that now.

@LexiestLeszek
Copy link
Contributor Author

I am fairly new to commiting to public libraries and don't understand what happend. How can I help?

@ZachNagengast
Copy link
Owner

@LexiestLeszek Nothing is needed from your end, just that github's machines don't have enough processing power to run the kind of benchmarks that we can run locally. I just reduced the processing time for one of the tests, so it should be ok to merge once the tests complete now.

Copy link
Owner

@ZachNagengast ZachNagengast left a comment

Choose a reason for hiding this comment

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

Build passed, looks good! Thanks for your PR, this is a great contribution 🙏

@ZachNagengast ZachNagengast merged commit 5f5b00a into ZachNagengast:main Jul 5, 2023
1 check passed
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.

2 participants