Skip to content

Conversation

@yu-iskw
Copy link
Contributor

@yu-iskw yu-iskw commented Jul 3, 2015

@SparkQA
Copy link

SparkQA commented Jul 3, 2015

Test build #36462 has finished for PR 7204 at commit 1737598.

  • 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 Jul 3, 2015

@shivaram Could you review this PR when you have time? Thanks!

R/pkg/R/utils.R Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call this storageLevelClass ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It is a just string variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but the variable name spelling is strange right now as strage -- could you make the variable name storage instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. I got it.

@SparkQA
Copy link

SparkQA commented Jul 3, 2015

Test build #36471 has finished for PR 7204 at commit 6fb131a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shivaram
Copy link
Contributor

shivaram commented Jul 3, 2015

Thanks @yu-iskw -- Just for my information can you paste a link to output of dev/lint-r after this change ?

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 6, 2015

@shivaram , Sure. That is the result of ./dev/lint-r at the revision d9838196ff48faeac19756852a7f695129c08047. Could you check it?
https://gist.github.com/yu-iskw/d3f414f2c18b2abc5766

@shivaram
Copy link
Contributor

shivaram commented Jul 6, 2015

Thanks @yu-iskw. BTW We should investigate how to get rid of the false warnings no visible global function definition as those functions are package private.

But this change LGTM. Merging this

@asfgit asfgit closed this in a0cb111 Jul 6, 2015
@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 6, 2015

@shivaram Thank you for merging it!
I agree with that. I will check the lintr code and ask Jim it.

@rxin
Copy link
Contributor

rxin commented Jul 6, 2015

@yu-iskw you might want to update your github email to include the email address you used for your commits. Otherwise it doesn't show up in the github metadata.

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 6, 2015

@rxin Thank you for letting me know. I have updated my github email in my account profile.

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