[SPARK-18901][ML]:Require in LR LogisticAggregator is redundant

[wangmiao1981]

What changes were proposed in this pull request?

In MultivariateOnlineSummarizer,

add and merge have check for weights and feature sizes. The checks in LR are redundant, which are removed from this PR.

How was this patch tested?

Existing tests.

[SparkQA]

Test build #75382 has finished for PR 17478 at commit 44305bf.

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

[sethah]

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 not always be the case. These checks are very small computation in comparison with the add and merge operations. I can imagine future developers finding that these are unchecked in the aggregator and trying to re-implement this logic.

If this were a performance fix, I'd support it. I don't think it is, and so I think the code is more readable and clear if we keep the checks.

[wangmiao1981]

@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 think just mark that JIRA as WON'T Fix? Thanks!

[sethah]

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 too strongly about it. Thoughts? Also, ping @hhbyyh

[hhbyyh]

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 relevant operation (especially add) could be invoked maybe billions of times (10M recoreds * 100 iterations) during a training. The check for feature dimension and weight should only be carried out once. But here they are conducted in each iteration.

And for the general code practice, I would not keep unnecessary code unless there's clear plan for it. That's just personal preference. cc @jkbradley

[sethah]

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 something because it's only used in conjunction with some other part of the code where it is checked does not seem a robust solution - at best it's quite unclear. I don't think it's a huge deal either way, but I'm in favor of keeping the checks.

[yanboliang]

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.

[yanboliang]

Merged into master and branch-2.2. Thanks for all.

[yanboliang]

@wangmiao1981 Would you mind to send a follow-up PR to address the same issue for LinearRegression and LinearSVC? Thanks.

[wangmiao1981]

@yanboliang I will do it. Thanks!