Skip to content

[SPARK-7663][MLlib] Add requirement for word2vec model #6228

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.

Sign up for GitHub

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

Closed
yinxusen wants to merge 2 commits into from
Closed

[SPARK-7663][MLlib] Add requirement for word2vec model #6228

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54ae63e
add requirement for word2vec model
yinxusen May 18, 2015
21770c5
check the vocab size
yinxusen May 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions mllib/src/main/scala/org/apache/spark/mllib/feature/Word2Vec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ class Word2Vec extends Serializable with Logging {
.sortWith((a, b) => a.cn > b.cn)

vocabSize = vocab.length
require(vocabSize > 0, "The vocabulary size should be large than 0. You may need to check " +
Copy link
Member

@jkbradley jkbradley May 18, 2015

Choose a reason for hiding this comment

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

Should this be a RuntimeException (rather than an IllegalArgumentException)?

Copy link
Contributor Author

@yinxusen yinxusen May 19, 2015

Choose a reason for hiding this comment

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

That's reasonable. Changed.

Copy link
Member

@srowen srowen May 19, 2015

Choose a reason for hiding this comment

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

Hm, I'd use require here. IllegalArgumentException is a RuntimeException, and I've always understood it to be slightly bad style to throw RuntimeException as you can't catch for it with catching for any RuntimeException.

Copy link
Member

@jkbradley jkbradley May 19, 2015

Choose a reason for hiding this comment

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

@srowen Do you have a better subclass of RuntimeException to suggest? I feel like there was no illegal argument given here. But looking at a list of subclasses [https://docs.oracle.com/javase/7/docs/api/java/lang/RuntimeException.html], I can't find one I like better.

Copy link
Member

@srowen srowen May 19, 2015

Choose a reason for hiding this comment

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

Yeah, this is real minor point of taste. I use IllegalStateException when there's no real argument to speak of, which is marginally more specific and description. But isn't this error caused by a bad input RDD? IllegalArgumentException seems like just the thing.

Copy link
Member

@jkbradley jkbradley May 19, 2015

Choose a reason for hiding this comment

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

Sounds good. @yinxusen Sorry for asking you to revert your change, but can you please stick with "require" per @srowen 's advice? Thank you!

Copy link
Contributor Author

@yinxusen yinxusen May 20, 2015

Choose a reason for hiding this comment

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

Never mind.

Copy link
Contributor Author

@yinxusen yinxusen May 20, 2015

Choose a reason for hiding this comment

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

@jkbradley I have reverted it back.

"the setting of minCount, which could be large enough to remove all your words in sentences.")

var a = 0
while (a < vocabSize) {
vocabHash += vocab(a).word -> a
Expand Down