[SPARK-9789] [ML] Added logreg threshold param back

[jkbradley]

Reinstated LogisticRegression.threshold Param for binary compatibility. Param thresholds overrides threshold, if set.

CC: @mengxr @dbtsai @feynmanliang

[feynmanliang]

Wy not provide implementation in trait since they are the same in the concrete classes?

[jkbradley]

I think it would require overriding still for Java compatibility. This way, we at least don't copy the doc.

[feynmanliang]

Just curious, any reason we have to mix in HasThreshold and physically keep the threshold around instead of just providing the threshold methods which wraps thresholds?

[jkbradley]

That's what I originally intended, but as Xiangrui pointed out, if users are using the Param threshold directly, then we'd break their code. This is better for legacy, though it complicates the API a bit.

[SparkQA]

Test build #40323 has finished for PR 8079 at commit 3d7501f.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[jkbradley]

jenkins test this please

[jkbradley]

jenkins test this please

[SparkQA]

Test build #40341 has finished for PR 8079 at commit fbf9e39.

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

[feynmanliang]

Makes sense, LGTM

[jkbradley]

Just realized I neglected PySpark. I'll add that since the 2 changes should go together.

[jkbradley]

@feynmanliang Would you mind taking another look? Thanks!

[SparkQA]

Test build #40359 has finished for PR 8079 at commit da7dd04.

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

[dbtsai]

Seems that we are preferring thresholds now, and I agree that it will be more flexible for multi-classes problem. However, users may set both of them with different values mistakenly, or for a old param, users use setThreshold, but now he switched to setThresholds which may cause some confusion. How about we throw Exception when they are not agreed? Also, whenever one is set, we just make the other unset.

[jkbradley]

I like failing whenever they disagree. I'll do that.

I'd prefer not to modify one when the other gets set. I think that's more confusing: If a user sets both, the user may not know what they are doing and should get a failure/warning. It's also unclear what the semantics should be if the user passes both Param values together in a single ParamMap.

[dbtsai]

LGTM except few comments. Thanks.

[jkbradley]

OK, that update should enforce equivalence when both are set.

[jkbradley]

jenkins test this please

[SparkQA]

Test build #40484 has finished for PR 8079 at commit 4ea4f2f.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dbtsai]

Where do we throw IllegalArgumentException?

[dbtsai]

I mean, the IllegalArgumentException is thrown when we checkThresholdConsistency not here. Should we document it that the exception will be thrown when the code is run not in the setting time?

[jkbradley]

Will do

[jkbradley]

Change of plans: I realized that, since we do not allow users to call Params.clear, I'll need to change the semantics to what you suggested: Whenever threshold or thresholds is set, it clears the other's value. (Otherwise, when fit is called with a threshold, then the resulting model cannot have its threshold changed very easily.)

[jkbradley]

Uh oh, my proposal won't work currently, but I think it can be fixed. Explanation here: [https://issues.apache.org/jira/browse/SPARK-9847]

[SparkQA]

Test build #40511 has finished for PR 8079 at commit 1a4b922.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • * Set thresholds in multiclass (or binary) classification to adjust the probability of

[dbtsai]

Then the most easy way is probably just overwriting the other when one is set. :)

[jkbradley]

That's true for the setter methods, but it doesn't work for fit() or transform() given ParamMaps. fit and transform don't use the setter methods, so we don't have a chance to unset the other parameter. I'll wait on this PR until the other patch gets merged.

[jkbradley]

Rebased... though I guess I could have merged. The copyValues fix should fix the unit tests.

[dbtsai]

LGTM. Waiting for tests

[SparkQA]

Test build #40651 has finished for PR 8079 at commit 8a0c3e0.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • * Set thresholds in multiclass (or binary) classification to adjust the probability of

[SparkQA]

Test build #40655 has finished for PR 8079 at commit af3d07a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • * Set thresholds in multiclass (or binary) classification to adjust the probability of

[jkbradley]

@dbtsai Thanks for checking this! I'll go ahead and merge it with master and branch-1.5