Allow TextStats length distribution to be token-based and refactor for testability #464
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,7 +140,7 @@ object TextTokenizer { | |
| /** | ||
| * Language wise sentence tokenization | ||
| * | ||
| * @param textString text to tokenize (in String form) | ||
| * @param languageDetector language detector instance | ||
| * @param analyzer text analyzer instance | ||
| * @param sentenceSplitter sentence splitter instance | ||
|
|
@@ -151,8 +151,8 @@ object TextTokenizer { | |
| * @param minTokenLength minimum token length | ||
| * @return detected language and sentence tokens | ||
| */ | ||
| private[op] def tokenizeString( | ||
| textString: String, | ||
|
There was a problem hiding this comment. now this function can explode with NullPointerException is textString is null, while before it could not have happened. There was a problem hiding this comment. Hmmm, I'm not sure if it's possible for the The actual tokenize call during vectorization is still I agree it's technically less safe, but I don't think it's necessary to have null checking at this point in the flow. We should make sure the data gets created in a safe way, which I think we already do. Are there some specific edge cases I'm missing? There was a problem hiding this comment. I think the simplest is to add a null check There was a problem hiding this comment. Now that I think about it more, I'm pretty sure the old tokenize function would also give a NPE if you fed it a sneaky null value. The I put back the old tokenize function as which did indeed throw a NPE. We have tests all over the place (eg. our vectorizer tests and FeatureTypeSparkConverterTest) that make sure we can handle @leahmcguire any opinions on this? There was a problem hiding this comment. SomeValue.unapply operates on value which is Option[String]. Null check is done during the construction of Text when the values are extracted from Dataframe / RDD. NullPointerException is indeed unlikely to be thrown. There was a problem hiding this comment. Your example you provided is not currently possible and also not a fair one :) There was a problem hiding this comment. @Jauntbox is this only called from the Option[String] version below? if so make it private and it is fine. There was a problem hiding this comment. In fact make them both private please |
||
| languageDetector: LanguageDetector = LanguageDetector, | ||
| analyzer: TextAnalyzer = Analyzer, | ||
| sentenceSplitter: Option[SentenceSplitter] = None, | ||
|
|
@@ -162,30 +162,84 @@ object TextTokenizer { | |
| toLowercase: Boolean = ToLowercase, | ||
| minTokenLength: Int = MinTokenLength | ||
| ): TextTokenizerResult = { | ||
| val language = | ||
| if (!autoDetectLanguage) defaultLanguage | ||
| else { | ||
| languageDetector | ||
| .detectLanguages(textString) | ||
| .collectFirst { case (lang, confidence) if confidence > autoDetectThreshold => lang } | ||
| .getOrElse(defaultLanguage) | ||
| } | ||
| val lowerTxt = if (toLowercase) textString.toLowerCase else textString | ||
|
|
||
| val sentences = sentenceSplitter.map(_.getSentences(lowerTxt, language)) | ||
| .getOrElse(Seq(lowerTxt)) | ||
| .map { sentence => | ||
| val tokens = analyzer.analyze(sentence, language) | ||
| tokens.filter(_.length >= minTokenLength).toTextList | ||
|
There was a problem hiding this comment. Why are we only keeping tokens with length > There was a problem hiding this comment. This was existing behavior. It's a configurable parameter (defaulting to 1), so is not required. |
||
| } | ||
| TextTokenizerResult(language, sentences) | ||
| } | ||
|
|
||
| /** | ||
| * Language wise sentence tokenization | ||
| * | ||
| * @param textStringOpt text to tokenize (in Option[String] form) | ||
| * @param languageDetector language detector instance | ||
| * @param analyzer text analyzer instance | ||
| * @param sentenceSplitter sentence splitter instance | ||
| * @param autoDetectLanguage whether to attempt language detection | ||
| * @param defaultLanguage default language | ||
| * @param autoDetectThreshold language detection threshold | ||
| * @param toLowercase whether to convert all characters to lowercase before tokenizing | ||
| * @param minTokenLength minimum token length | ||
| * @return detected language and sentence tokens | ||
| */ | ||
| private[op] def tokenizeStringOpt( | ||
| textStringOpt: Option[String], | ||
| languageDetector: LanguageDetector = LanguageDetector, | ||
| analyzer: TextAnalyzer = Analyzer, | ||
| sentenceSplitter: Option[SentenceSplitter] = None, | ||
| autoDetectLanguage: Boolean = AutoDetectLanguage, | ||
| defaultLanguage: Language = DefaultLanguage, | ||
| autoDetectThreshold: Double = AutoDetectThreshold, | ||
| toLowercase: Boolean = ToLowercase, | ||
| minTokenLength: Int = MinTokenLength | ||
| ): TextTokenizerResult = { | ||
| textStringOpt match { | ||
| case Some(txt) => tokenizeString(txt, languageDetector, analyzer, sentenceSplitter, autoDetectLanguage, | ||
| defaultLanguage, autoDetectThreshold, toLowercase, minTokenLength) | ||
| case None => TextTokenizerResult(defaultLanguage, Seq(TextList.empty)) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Language wise sentence tokenization | ||
| * | ||
| * @param text text to tokenize | ||
| * @param languageDetector language detector instance | ||
| * @param analyzer text analyzer instance | ||
| * @param sentenceSplitter sentence splitter instance | ||
| * @param autoDetectLanguage whether to attempt language detection | ||
| * @param defaultLanguage default language | ||
| * @param autoDetectThreshold language detection threshold | ||
| * @param toLowercase whether to convert all characters to lowercase before tokenizing | ||
| * @param minTokenLength minimum token length | ||
| * @return detected language and sentence tokens | ||
| */ | ||
| def tokenize( | ||
tovbinm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| text: Text, | ||
| languageDetector: LanguageDetector = LanguageDetector, | ||
| analyzer: TextAnalyzer = Analyzer, | ||
| sentenceSplitter: Option[SentenceSplitter] = None, | ||
| autoDetectLanguage: Boolean = AutoDetectLanguage, | ||
| defaultLanguage: Language = DefaultLanguage, | ||
| autoDetectThreshold: Double = AutoDetectThreshold, | ||
| toLowercase: Boolean = ToLowercase, | ||
| minTokenLength: Int = MinTokenLength | ||
| ): TextTokenizerResult = tokenizeStringOpt(text.value, languageDetector, analyzer, sentenceSplitter, | ||
| autoDetectLanguage, defaultLanguage, autoDetectThreshold, toLowercase, minTokenLength) | ||
|
|
||
| /** | ||
| * Text tokenization result | ||
| * | ||
|
|
||
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
shouldCleanText = trueinstead ?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.
I can change it, but I don't think it matters much here. These values aren't used in SmartTextVectorizer. They're the ones that show up in the ModelInsights.