Skip to content
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

[SPARK-7104][MLlib] Support model save/load in Python's Word2Vec #6821

Closed
wants to merge 4 commits into from

Conversation

yu-iskw
Copy link
Contributor

@yu-iskw yu-iskw commented Jun 15, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Jun 15, 2015

Test build #34911 has finished for PR 6821 at commit 562edfc.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class Word2VecModel(JavaVectorTransformer, JavaSaveable, JavaLoader):

@SparkQA
Copy link

SparkQA commented Jun 15, 2015

Test build #34915 has finished for PR 6821 at commit 7fb37eb.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class Word2VecModel(JavaVectorTransformer, JavaSaveable, JavaLoader):

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jun 20, 2015

@jkbradley Could you review it when you have time? Thanks!

>>> path = tempfile.mkdtemp()
>>> model.save(sc, path)
>>> sameModel = Word2VecModel.load(sc, path)
>>> model.transform("a") == sameModel.transform("a")
Copy link
Contributor

Choose a reason for hiding this comment

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

It has been advised by @mengxr to keep doctests and testing for correctness separately. (#6499 (comment))
It might be better to just write sameModel.transform("a") and write a small test to test model and sameModel give same results using getVectors maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I meant was this test is not for correctness. I wanted to check whether a saved model works or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but I think it would be better to just write
sameModel.transform(a) separately and the output.

However, I have no objections either !

@jkbradley
Copy link
Member

will make a pass! test this please

@jkbradley
Copy link
Member

Looks good except for the pending comments

@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35587 has finished for PR 6821 at commit 7fb37eb.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class Word2VecModel(JavaVectorTransformer, JavaSaveable, JavaLoader):

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jun 24, 2015

Jenkins, test this please.

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35610 has finished for PR 6821 at commit 7fb37eb.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class Word2VecModel(JavaVectorTransformer, JavaSaveable, JavaLoader):

@@ -27,6 +27,7 @@ import scala.language.existentials
import scala.reflect.ClassTag

import net.razorvine.pickle._
import org.apache.spark.SparkContext
Copy link
Member

Choose a reason for hiding this comment

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

organize imports

@jkbradley
Copy link
Member

@yu-iskw The only pending comments I see are the one I just made (organize imports) + using rmtree in the doctest. After those, this should be ready.

@SparkQA
Copy link

SparkQA commented Jul 2, 2015

Test build #36381 has finished for PR 6821 at commit 975136b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class Word2VecModel(JavaVectorTransformer, JavaSaveable, JavaLoader):

@jkbradley
Copy link
Member

LGTM merging with master
Thanks!

@asfgit asfgit closed this in 488bad3 Jul 2, 2015
@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 2, 2015

@jkbradley Thank you for merging it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants