Skip to content

Adding NGram range support for feature extraction#2

Closed
madhawa-gunasekara wants to merge 3 commits into
apache:trunkfrom
madhawa-gunasekara:OPENNLP-844
Closed

Adding NGram range support for feature extraction#2
madhawa-gunasekara wants to merge 3 commits into
apache:trunkfrom
madhawa-gunasekara:OPENNLP-844

Conversation

@madhawa-gunasekara

Copy link
Copy Markdown
Member

@chrismattmann

Copy link
Copy Markdown
Contributor

+1 to merge

@kelkarn

kelkarn commented Apr 4, 2016

Copy link
Copy Markdown

It would be nice to either document behavior, or handle the case where minGram > maxGram (if passed by user mistakenly, for example).

@chrismattmann

Copy link
Copy Markdown
Contributor

good validation step @kelkarn

@madhawa-gunasekara

Copy link
Copy Markdown
Member Author

@kelkarn I will update the PR request. Thanks

this.maxGram = maxGram;
} else {
throw new InvalidFormatException("minimum range value (minGram) should be less than or equal to maximum range value (maxGram)!");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is good, but what if minGram/maxGram are negative?

@beylerian

Copy link
Copy Markdown

Hey there, we could probably deprecate bag of words and use this one with unigrams
Also please keep the space separator for backward compatibility

feature = feature + ":" + text[i + y];
feature = feature + " " + text[i + y];

@madhawa-gunasekara

Copy link
Copy Markdown
Member Author

Closing pull request
Since patch has applied.

maximestein pushed a commit to maximestein/opennlp that referenced this pull request Jan 7, 2019
…ool-close-failure-in-OpenNLP-GIS-class

Feature tec 1918 threadpool close failure in open nlp gis class
klu2300031264 referenced this pull request in klu2300031264/opennlp Apr 16, 2025
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.

4 participants