[SPARK-5893][ML] Add bucketizer

[yinxusen]

JIRA issue here.

One thing to make clear, the buckets parameter, which is an array of Double, performs as split points. Say,

buckets = Array(-0.5, 0.0, 0.5)

splits the real number into 4 ranges, (-inf, -0.5], (-0.5, 0.0], (0.0, 0.5], (0.5, +inf), which is encoded as 0, 1, 2, 3.

[AmplabJenkins]

Merged build triggered.

[yinxusen]

@jkbradley

[AmplabJenkins]

Merged build started.

[SparkQA]

Test build #32110 has started for PR 5980 at commit 998bc87.

[yinxusen]

@jkbradley, I will push the refactored #5779 as soon as this PR merged.

[AmplabJenkins]

Merged build finished. Test FAILed.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32110/
Test FAILed.

[shaneknapp]

i'll retrigger this build once jenkins is back up...

[AmplabJenkins]

Merged build triggered.

[yinxusen]

I change the Bucketizer from a Transform to a Model, otherwise we can not use it as the output of an Estimator.

[AmplabJenkins]

Merged build started.

[shaneknapp]

jenkins, test this please

[SparkQA]

Test build #32114 has started for PR 5980 at commit 11fb00a.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[SparkQA]

Test build #32115 has started for PR 5980 at commit 11fb00a.

[SparkQA]

Test build #32114 has finished for PR 5980 at commit 11fb00a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • final class Bucketizer(override val parent: Estimator[Bucketizer] = null)

[AmplabJenkins]

Merged build finished. Test PASSed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32114/
Test PASSed.

[SparkQA]

Test build #32115 has finished for PR 5980 at commit 11fb00a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • final class Bucketizer(override val parent: Estimator[Bucketizer] = null)

[AmplabJenkins]

Merged build finished. Test PASSed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32115/
Test PASSed.

[jkbradley]

This should use 2 constructors (1 with an argument, 1 without) to be Java-friendly. Also, can we make the constructor with 1 argument be private[ml] for now?

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32193/
Test PASSed.

[mengxr]

@yinxusen @jkbradley Instead of having three parameters to control the buckets, is it simpler to let the users provide the boundaries directly? For example, [0, 1, 2, ..., 24] gives 24 buckets. If users want to include values out of this range, they should provide [-inf, 0, 1, 2, 3, ..., 24, inf] instead. This would also simplify the implementation.

[yinxusen]

@mengxr How to represent -inf and inf? One possible solution is

val inf = Double.MaxValue
val negInf = Double.MinValue
splits = Array(negInf, 0, 1, 2, 3, inf)

Or we can define inf and negInf as double parameters, without setter.

val inf = new DoubleParam(this, "inf", "")
setDefault(inf -> Double.MaxValue)
def getInf = $(inf)
val negInf = new DoubleParam(this, "negInf", "")
setDefault(negInf -> Double.MinValue)
def getNegInf = $(negInf)

However, it would break the meaning of splits, I think.

[mengxr]

inf is in the IEEE standard for floating-point numbers:

• Java Double.POSITIVE_INFINITY: https://docs.oracle.com/javase/7/docs/api/java/lang/Double.html#POSITIVE_INFINITY
• Scala Double.PositiveInfinity: http://www.scala-lang.org/api/current/index.html#scala.Double$
• Python float('inf'): https://docs.python.org/2/library/functions.html#float

[jkbradley]

@mengxr I thought about this, but here's the comment I posted above:

One issue was brought up: What about ranges of buckets where the ends (-inf, firstSplit] and (lastSplit, inf) don't make sense? E.g., for ages, we don't want a range (-inf, 0]. We could include this in a few ways:

(1) Make users include -inf and/or inf in the splits if they want those ranges.
(2) Have Boolean parameters lowerInclusive, upperInclusive to indicate whether to include those ranges.

I prefer the Boolean parameters with defaults of true. That will make it easy to have the default behavior accept all numerical values. When the range does not to to -inf and inf, then we'll have to throw an error and report the bad value.

[mengxr]

Sorry that I didn't see the hidden GitHub comments. I think it depends on the use cases of bucketizer. My guess is that in most cases users use bucketizer on values that they already know the range. For example, time -> hourOfDay, dayOfMonth, or age -> 10-year buckets. In those cases, accepting (-inf, firstSplit] and (lastSplit, inf) by default may result empty buckets. Another issue is what the expected behavior is for values out of range. Should they go to a new bucket, or be merged into the closest bucket? Letting users explicitly specify the buckets would be able to avoid those problems.

[jkbradley]

It's hard to say for use cases. It's true empty buckets might be a problem. I'm OK with requiring explicit buckets for -inf, inf; let's just do that, but we'll have to make sure we document that in the Scala doc + examples. For values out of range, there are several behaviors the user might want:

• new bucket for all bad values
• closest bucket
• throw an error (to make user aware of bad data or assumptions)

We could eventually support all of these via a new parameter, but for now, shall we choose 1? I suppose I'd vote for putting values in the closest bucket (which will mean the first and last split values will be changed to -inf, inf under the hood).

[mengxr]

Yes, we need document all cases. I kinda like 3 (throw an error) as the default, because it is the most strict. If there is an exception, users should decide to switch to 1 or 2 by changing the bucket boundaries.

[jkbradley]

@yinxusen OK, so I think we've converged. Can you please update it to take splits, but no "lowerInclusive, upperInclusive" parameters? Let's start with 3 (throw an error) for handling bad values. It will be great if the driver catches the SparkException and prints a more useful error message, recommending the user set the bin boundaries to -inf, inf.

A later PR can add an extra parameter for supporting all 3 behaviors.

[yinxusen]

@mengxr @jkbradley I see. I will be soon, say, in 2 days. It's really a little busy here recently. Sorry for the delay.

[jkbradley]

@yinxusen I'm worried about this missing 1.4. Would you mind if I sent a PR to you to update this PR?

[yinxusen]

@jkbradley Yes, thanks! Sorry for the delay again.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[SparkQA]

Test build #32430 has started for PR 5980 at commit dc8c843.

[yinxusen]

@jkbradley I have merged your PR. Thanks!

[jkbradley]

@yinxusen Thanks! I'll merge once tests pass

[SparkQA]

Test build #32430 has finished for PR 5980 at commit dc8c843.

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

[AmplabJenkins]

Merged build finished. Test PASSed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32430/
Test PASSed.

[jkbradley]

Merging into master and branch-1.4. @yinxusen Thanks!