You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by AiHe <gi...@git.apache.org> on 2015/04/24 12:28:39 UTC
[GitHub] spark pull request: [Minor][MLLIB] Fix a formatting bug in toStrin...
GitHub user AiHe opened a pull request:
https://github.com/apache/spark/pull/5687
[Minor][MLLIB] Fix a formatting bug in toString method in Node
1. predict(predict.toString) has already output prefix “predict” thus it’s duplicated to print ", predict = " again
2. there are some extra spaces
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/AiHe/spark tree-node-issue-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/5687.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 #5687
----
commit 426eee7fa00eef343d10396704f0619e802841bc
Author: Alain <ai...@usc.edu>
Date: 2015-04-24T09:26:03Z
[Minor][MLLIB] Fix a formatting bug in toString method in Node.scala
1. predict(predict.toString) has already output prefix “predict” thus
it’s
duplicate to print ", predict = " again
2. there are some extra spaces
----
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96245388
ok to test
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by AiHe <gi...@git.apache.org>.
Github user AiHe commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96102493
Okay, sounds better to use a modern style.
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by AiHe <gi...@git.apache.org>.
Github user AiHe commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96058819
If you're referring to the modification on Predict.scala, I just follow the style of previous code.
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96344428
[Test build #710 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/710/consoleFull) for PR 5687 at commit [`9862b9a`](https://github.com/apache/spark/commit/9862b9a716f1afdff384f5d155846b4c99683b9d).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96282678
LGTM pending tests
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/5687
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96103225
... this still is not using string interpolation everywhere. It's a small thing, but if we're bothering to update these `toString` methods, go all the way.
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by AiHe <gi...@git.apache.org>.
Github user AiHe commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96133857
Get your point and change all toString methods in MLLIB.
There are a large number of uses of 'old' way in the statement like logInfo, which makes it hard to change all them in a moment.
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96066332
Yes, but that's a hold over from Scala 2.9 and before. I think that while we're modifying these strings they can use the 'modern' style used in new code in the project.
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by AiHe <gi...@git.apache.org>.
Github user AiHe commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96234324
Good to go.
@jkbradley Thank you.
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by AiHe <gi...@git.apache.org>.
Github user AiHe commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96055725
Modify Predict.scala and decouple the dependency of Node.scala on Predict.scala.
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96286191
[Test build #699 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/699/consoleFull) for PR 5687 at commit [`b6380c6`](https://github.com/apache/spark/commit/b6380c6770a913efe07c0f8fd8621843cced9fc4).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch **adds the following new dependencies:**
* `tachyon-0.6.4.jar`
* `tachyon-client-0.6.4.jar`
* This patch **removes the following dependencies:**
* `tachyon-0.5.0.jar`
* `tachyon-client-0.5.0.jar`
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/5687#discussion_r29095837
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/model/Predict.scala ---
@@ -29,9 +29,7 @@ class Predict(
val predict: Double,
val prob: Double = 0.0) extends Serializable {
- override def toString: String = {
- "predict = %f, prob = %f".format(predict, prob)
- }
+ override def toString: String = s"$predict(prob = $prob)"
--- End diff --
I'd put a space before the parenthesis: ```$predict (prob```
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96286267
@AiHe this fails style checks. I think the lines are too long. Can you fix those up and try `./dev/lint-scala` to check it out locally?
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96282719
[Test build #699 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/699/consoleFull) for PR 5687 at commit [`b6380c6`](https://github.com/apache/spark/commit/b6380c6770a913efe07c0f8fd8621843cced9fc4).
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by AiHe <gi...@git.apache.org>.
Github user AiHe commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96318888
@srowen Sure. Pass the style checking and will do that for PRs in the feature.
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96056182
I'm OK with this, though while you're at it, why not make these use string interpolation? `s"foo = $foo, ..."`
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96335555
[Test build #710 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/710/consoleFull) for PR 5687 at commit [`9862b9a`](https://github.com/apache/spark/commit/9862b9a716f1afdff384f5d155846b4c99683b9d).
---
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: [Minor][MLLIB] Refactor toString method in MLL...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-96191595
Looks OK, I think Joseph just had one more tiny comment on one `toString` method, to add a space.
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5687#discussion_r29041932
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/model/Node.scala ---
@@ -51,8 +51,8 @@ class Node (
var stats: Option[InformationGainStats]) extends Serializable with Logging {
override def toString: String = {
- "id = " + id + ", isLeaf = " + isLeaf + ", predict = " + predict + ", " +
--- End diff --
These can use string interpolation. I take your point though it breaks the symmetry a bit and make this `toString` rely on details of the subclass. How about making the `Predict.toString` return something more compact like `s"$predict ($prob)"`?
---
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: [Minor][MLLIB] Fix a formatting bug in toStrin...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5687#issuecomment-95884593
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