[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-04-24 Thread wangmiao1981
Github user wangmiao1981 commented on the issue: https://github.com/apache/spark/pull/17478 @yanboliang I will do it. 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

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-04-24 Thread yanboliang
Github user yanboliang commented on the issue: https://github.com/apache/spark/pull/17478 @wangmiao1981 Would you mind to send a follow-up PR to address the same issue for ```LinearRegression``` and ```LinearSVC```? Thanks. --- If your project is set up for it, you can reply to this

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-04-24 Thread yanboliang
Github user yanboliang commented on the issue: https://github.com/apache/spark/pull/17478 Merged into master and branch-2.2. Thanks for all. --- 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

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-04-24 Thread yanboliang
Github user yanboliang commented on the issue: https://github.com/apache/spark/pull/17478 I agree with @hhbyyh in this case. The check for feature dimension should only be carried out once rather than running in each iteration, and actually we have checked this before iterations.

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread sethah
Github user sethah commented on the issue: https://github.com/apache/spark/pull/17478 Checking the size is a constant time operation, but in `add` we do also do a linear time dot product. I do not think this affects performance. I don't exactly mind removing it, but not checking

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread hhbyyh
Github user hhbyyh commented on the issue: https://github.com/apache/spark/pull/17478 Thanks for @wangmiao1981 for the PR and @sethah for the comments. Maybe I should be more clear when I created the jira. I would prefer to remove the require here permanently. The

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread sethah
Github user sethah commented on the issue: https://github.com/apache/spark/pull/17478 Yeah, I would support adding a unit test to the logistic aggregator (well, all aggregators) for these types of things. I do think it's better to keep them and add a couple tests, but I don't feel

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread wangmiao1981
Github user wangmiao1981 commented on the issue: https://github.com/apache/spark/pull/17478 @sethah Thanks for your reply! Your suggestion makes sense to me. My intention was to close the JIRA by simple fix. How about we add a test for these checks and close the original JIRA? or you

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread sethah
Github user sethah commented on the issue: https://github.com/apache/spark/pull/17478 I guess I don't see the harm in keeping these checks. Yes, in this case we always call `LogisticAggregator` after we have gone through the same data with `MultivariateOnlineSummarizer`, but it may

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread AmplabJenkins
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/17478 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

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread AmplabJenkins
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/17478 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/75382/ Test PASSed. ---

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread SparkQA
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/17478 **[Test build #75382 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75382/testReport)** for PR 17478 at commit

[GitHub] spark issue #17478: [SPARK-18901][ML]:Require in LR LogisticAggregator is re...

2017-03-30 Thread SparkQA
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/17478 **[Test build #75382 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75382/testReport)** for PR 17478 at commit