You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by yanboliang <gi...@git.apache.org> on 2016/11/09 08:42:17 UTC
[GitHub] spark pull request #15826: [SPARK-14077][ML][FOLLOW-UP] Minor cleanups for N...
GitHub user yanboliang opened a pull request:
https://github.com/apache/spark/pull/15826
[SPARK-14077][ML][FOLLOW-UP] Minor cleanups for NaiveBayes
## What changes were proposed in this pull request?
* Refactor out ```trainWithLabelCheck``` and make ```mllib.NaiveBayes``` call into it.
* Avoid capturing the outer object for ```modelType```.
* Move ```requireNonnegativeValues``` and ```requireZeroOneBernoulliValues``` to companion object.
## How was this patch tested?
Existing tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yanboliang/spark spark-14077-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/15826.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #15826
----
commit d0f95fb9a4696f1932febd3e58501ddb8f14555a
Author: Yanbo Liang <yb...@gmail.com>
Date: 2016-11-09T07:16:50Z
Refactor out trainWithLabelCheck.
commit 8cf2a80e2b3df2038387aa50c03b628ce26d70b0
Author: Yanbo Liang <yb...@gmail.com>
Date: 2016-11-09T07:46:20Z
Avoid capturing the outer object.
commit 48f7e868cad81f49ca6206031572424fa15ce74d
Author: Yanbo Liang <yb...@gmail.com>
Date: 2016-11-09T08:35:46Z
Update some variables to more private.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and c...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/15826#discussion_r87152501
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/NaiveBayes.scala ---
@@ -226,13 +206,33 @@ class NaiveBayes @Since("1.5.0") (
@Since("1.6.0")
object NaiveBayes extends DefaultParamsReadable[NaiveBayes] {
/** String name for multinomial model type. */
- private[spark] val Multinomial: String = "multinomial"
+ private[classification] val Multinomial: String = "multinomial"
/** String name for Bernoulli model type. */
- private[spark] val Bernoulli: String = "bernoulli"
+ private[classification] val Bernoulli: String = "bernoulli"
/* Set of modelTypes that NaiveBayes supports */
- private[spark] val supportedModelTypes = Set(Multinomial, Bernoulli)
+ private[classification] val supportedModelTypes = Set(Multinomial, Bernoulli)
+
+ private[NaiveBayes] val requireNonnegativeValues: Vector => Unit = (v: Vector) => {
--- End diff --
@thunterdb I define these two functions still by ```val``` which should be more appropriate, since there is difference between ```val``` and ```def``` to define a function. http://stackoverflow.com/questions/18887264/what-is-the-difference-between-def-and-val-to-define-a-function
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15826
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/68398/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15826
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/15826
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15826
**[Test build #68443 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68443/consoleFull)** for PR 15826 at commit [`89d0c77`](https://github.com/apache/spark/commit/89d0c774c1ae5df1c7cea4feb6b970e2fa0b8cb1).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by thunterdb <gi...@git.apache.org>.
Github user thunterdb commented on the issue:
https://github.com/apache/spark/pull/15826
@yanboliang that looks great, thank you. LGTM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/15826
@thunterdb Any more comments? Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and c...
Posted by thunterdb <gi...@git.apache.org>.
Github user thunterdb commented on a diff in the pull request:
https://github.com/apache/spark/pull/15826#discussion_r87256910
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/NaiveBayes.scala ---
@@ -110,21 +110,20 @@ class NaiveBayes @Since("1.5.0") (
@Since("2.1.0")
def setWeightCol(value: String): this.type = set(weightCol, value)
+ override protected def train(dataset: Dataset[_]): NaiveBayesModel = {
+ trainWithLabelCheck(dataset)
+ }
+
/**
* ml assumes input labels in range [0, numClasses). But this implementation
* is also called by mllib NaiveBayes which allows other kinds of input labels
- * such as {-1, +1}. Here we use this parameter to switch between different processing logic.
- * It should be removed when we remove mllib NaiveBayes.
+ * such as {-1, +1}. `positiveLabel` is used to determine whether the label
+ * should be checked and it should be removed when we remove mllib NaiveBayes.
*/
- private[spark] var isML: Boolean = true
-
- private[spark] def setIsML(isML: Boolean): this.type = {
- this.isML = isML
- this
- }
-
- override protected def train(dataset: Dataset[_]): NaiveBayesModel = {
- if (isML) {
+ private[spark] def trainWithLabelCheck(
+ dataset: Dataset[_],
+ positiveLabel: Boolean = true): NaiveBayesModel = {
--- End diff --
Since it is an internal method, I suggest to not use a default argument and specify it explicitly above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15826
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15826
**[Test build #68443 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68443/consoleFull)** for PR 15826 at commit [`89d0c77`](https://github.com/apache/spark/commit/89d0c774c1ae5df1c7cea4feb6b970e2fa0b8cb1).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/15826
Merged into master and branch-2.1. Thanks for reviewing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and c...
Posted by thunterdb <gi...@git.apache.org>.
Github user thunterdb commented on a diff in the pull request:
https://github.com/apache/spark/pull/15826#discussion_r87256589
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/NaiveBayes.scala ---
@@ -226,13 +206,33 @@ class NaiveBayes @Since("1.5.0") (
@Since("1.6.0")
object NaiveBayes extends DefaultParamsReadable[NaiveBayes] {
/** String name for multinomial model type. */
- private[spark] val Multinomial: String = "multinomial"
+ private[classification] val Multinomial: String = "multinomial"
/** String name for Bernoulli model type. */
- private[spark] val Bernoulli: String = "bernoulli"
+ private[classification] val Bernoulli: String = "bernoulli"
/* Set of modelTypes that NaiveBayes supports */
- private[spark] val supportedModelTypes = Set(Multinomial, Bernoulli)
+ private[classification] val supportedModelTypes = Set(Multinomial, Bernoulli)
+
+ private[NaiveBayes] val requireNonnegativeValues: Vector => Unit = (v: Vector) => {
--- End diff --
@yanboliang indeed it makes a difference when defining methods inside another method. In the current case, you brought the function as a value of the companion object. You can safely turn it into a method
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15826
**[Test build #68398 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68398/consoleFull)** for PR 15826 at commit [`48f7e86`](https://github.com/apache/spark/commit/48f7e868cad81f49ca6206031572424fa15ce74d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15826
**[Test build #68398 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/68398/consoleFull)** for PR 15826 at commit [`48f7e86`](https://github.com/apache/spark/commit/48f7e868cad81f49ca6206031572424fa15ce74d).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15826
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/68443/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and cleanup ...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/15826
cc @thunterdb @zhengruifeng
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15826: [SPARK-14077][ML][FOLLOW-UP] Minor refactor and c...
Posted by thunterdb <gi...@git.apache.org>.
Github user thunterdb commented on a diff in the pull request:
https://github.com/apache/spark/pull/15826#discussion_r87256702
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/NaiveBayes.scala ---
@@ -226,13 +206,33 @@ class NaiveBayes @Since("1.5.0") (
@Since("1.6.0")
object NaiveBayes extends DefaultParamsReadable[NaiveBayes] {
/** String name for multinomial model type. */
- private[spark] val Multinomial: String = "multinomial"
+ private[classification] val Multinomial: String = "multinomial"
/** String name for Bernoulli model type. */
- private[spark] val Bernoulli: String = "bernoulli"
+ private[classification] val Bernoulli: String = "bernoulli"
/* Set of modelTypes that NaiveBayes supports */
- private[spark] val supportedModelTypes = Set(Multinomial, Bernoulli)
+ private[classification] val supportedModelTypes = Set(Multinomial, Bernoulli)
+
+ private[NaiveBayes] val requireNonnegativeValues: Vector => Unit = (v: Vector) => {
+ val values = v match {
+ case sv: SparseVector => sv.values
+ case dv: DenseVector => dv.values
+ }
+
+ require(values.forall(_ >= 0.0),
+ s"Naive Bayes requires nonnegative feature values but found $v.")
+ }
+
+ private[NaiveBayes] val requireZeroOneBernoulliValues: Vector => Unit = (v: Vector) => {
--- End diff --
same thing here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org