-
Notifications
You must be signed in to change notification settings - Fork 247
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
feat: Add SpaCy as a pre-splitter for Rolling Window - Fix #193 #204
base: main
Are you sure you want to change the base?
Conversation
klein-t
commented
Mar 15, 2024
- add SpaCy as an option for pre-splitting;
- add check for "en_core_web_sm" pipeline: if not present, install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @klein-t — thanks for the PR this is a great idea, I have just some small feedback points on making spacy
an optional dependency before we merge — lmk what you think, thanks!
Hey @jamescalam, all made sense. I implemented the changes. Let me know if all is good, I was really short on time so I did things in the quickest way I could think of. All is tested, but still, let me know how you feel about it. Cheers, Klein |
Fixing a small bug
Hey @klein-t please fix the poetry lock issue and add the tests that you mentioned to the test_splitters.py file. Additionally, some tests that can be added:
Besides that, everything works correctly. |
Hey @mesax1, thanks. I will do it later today/tomorrow! |