-
Notifications
You must be signed in to change notification settings - Fork 777
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
Fixed Issue: #1977 #2181
Fixed Issue: #1977 #2181
Changes from 3 commits
2601ac3
ede4d99
7796fbc
19bab3d
aa03a25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import random | ||
import time | ||
from typing import Union | ||
|
||
|
||
def truncate_document(topic_model, doc_length, tokenizer, document: str): | ||
def truncate_document(topic_model, doc_length: Union[int, None], tokenizer: Union[str, callable], document: str) -> str: | ||
"""Truncate a document to a certain length. | ||
|
||
If you want to add a custom tokenizer, then it will need to have a `decode` and | ||
|
@@ -57,6 +58,21 @@ def decode(self, doc_chunks): | |
return truncated_document | ||
return document | ||
|
||
def validate_truncate_document_parameters(tokenizer, doc_length) -> Union[None, ValueError]: | ||
"""validates parameters that are used in the function `truncate_document`""" | ||
if tokenizer is None and doc_length is not None: | ||
raise ValueError( | ||
"Please select from one of the valid options for the `tokenizer` parameter: \n" | ||
"{'char', 'whitespace', 'vectorizer'} \n" | ||
"If `tokenizer` is of type callable ensure it has methods to encode and decode a document \n" | ||
) | ||
elif tokenizer is not None and doc_length is None: | ||
raise ValueError( | ||
"If `tokenizer` is provided, `doc_length` of type int must be provided as well." | ||
) | ||
else: | ||
pass | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this isn't necessary right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it is not necessary but I included it to be verbose in terms of the possible edgecase. That being said, happy to remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove it and I'll make sure to merge it after that! Other than that, it looks great! |
||
|
||
|
||
def retry_with_exponential_backoff( | ||
func, | ||
|
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.
Should we also include a check for the opposite? That someone does use a tokenizer but not a doc_length?