You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rerngvit <gi...@git.apache.org> on 2015/09/23 11:04:24 UTC
[GitHub] spark pull request: [SPARK-9798] [ML] CrossValidatorModel Document...
GitHub user rerngvit opened a pull request:
https://github.com/apache/spark/pull/8882
[SPARK-9798] [ML] CrossValidatorModel Documentation Improvements
Document CrossValidatorModel members: bestModel and avgMetrics
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rerngvit/spark Spark-9798
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/8882.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 #8882
----
commit 08835af8282d310d7692f30578fb69b40a5b3507
Author: Rerngvit Yanggratoke <re...@kth.se>
Date: 2015-09-23T08:37:48Z
Add documentation for CrossValidatorModel members: bestModel and avgMetrics
----
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by rerngvit <gi...@git.apache.org>.
Github user rerngvit commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-144946863
@jkbradley Thank you for your review. I updated the doc for avgMetrics according to your comment. Please have a look.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/8882#discussion_r40275073
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tuning/CrossValidator.scala ---
@@ -140,7 +140,11 @@ class CrossValidator(override val uid: String) extends Estimator[CrossValidatorM
@Experimental
class CrossValidatorModel private[ml] (
override val uid: String,
+
+ /** The best model selected from cross validation */
--- End diff --
See other classes for examples.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-145189050
@rerngvit no worry
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-145114370
fyi, the hot fix has been merged.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by rerngvit <gi...@git.apache.org>.
Github user rerngvit commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-145187946
@yhuai Sorry for that.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/8882#discussion_r40275011
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tuning/CrossValidator.scala ---
@@ -140,7 +140,11 @@ class CrossValidator(override val uid: String) extends Estimator[CrossValidatorM
@Experimental
class CrossValidatorModel private[ml] (
override val uid: String,
+
+ /** The best model selected from cross validation */
--- End diff --
These should be documented in the class Scala doc (line 138+), using the ```@param``` tag.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-145121813
Rats, I'm really sorry. I didn't realize tests hadn't run.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-144889179
Thanks, this should work, pending that 1 doc update.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/8882
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by rerngvit <gi...@git.apache.org>.
Github user rerngvit commented on a diff in the pull request:
https://github.com/apache/spark/pull/8882#discussion_r40483611
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tuning/CrossValidator.scala ---
@@ -140,7 +140,11 @@ class CrossValidator(override val uid: String) extends Estimator[CrossValidatorM
@Experimental
class CrossValidatorModel private[ml] (
override val uid: String,
+
+ /** The best model selected from cross validation */
--- End diff --
@jkbradley Thank you for your clarification. I modified the PR according to your comment. Please have a look. Note that I also rebase to the master branch, since it has been moved.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-142535816
Can one of the admins verify this patch?
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by rerngvit <gi...@git.apache.org>.
Github user rerngvit commented on a diff in the pull request:
https://github.com/apache/spark/pull/8882#discussion_r40295753
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tuning/CrossValidator.scala ---
@@ -140,7 +140,11 @@ class CrossValidator(override val uid: String) extends Estimator[CrossValidatorM
@Experimental
class CrossValidatorModel private[ml] (
override val uid: String,
+
+ /** The best model selected from cross validation */
--- End diff --
@jkbradley I don't fully understand. As far as I understand, @param is for documenting arguments of methods. However, both bestModel and avgMetrics (that Spark-9798 are about) are not arguments in any methods (including constructors). Why should there be @params for the two members for the class? Could you please help elaborate?
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-145113338
Seems this one breaks our build.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-145113841
For doc-only change, it will be good to run the style check.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/8882#discussion_r40356193
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tuning/CrossValidator.scala ---
@@ -140,7 +140,11 @@ class CrossValidator(override val uid: String) extends Estimator[CrossValidatorM
@Experimental
class CrossValidatorModel private[ml] (
override val uid: String,
+
+ /** The best model selected from cross validation */
--- End diff --
Those are params for the class constructor. I'd recommend you try generating the Scala docs as you have the code now, and with the docs moved to param docs, to see the difference.
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/8882#discussion_r40983631
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tuning/CrossValidator.scala ---
@@ -136,6 +136,9 @@ class CrossValidator(override val uid: String) extends Estimator[CrossValidatorM
/**
* :: Experimental ::
* Model from k-fold cross validation.
+ *
+ * @param bestModel The best model selected from k-fold cross validation.
+ * @param avgMetrics Average cross-validation metrics.
--- End diff --
Augment:
```Average cross-validation metrics for each paramMap in [[estimatorParamMaps]], in the corresponding order."
---
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: [SPARK-9798] [ML] CrossValidatorModel Document...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/8882#issuecomment-145089089
LGTM, thanks! Merging with master
---
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