[SPARK-17086][ML] Fix InvalidArgumentException issue in QuantileDiscretizer when some quantiles are duplicated

[VinceShieh]

What changes were proposed in this pull request?

In cases when QuantileDiscretizerSuite is called upon a numeric array with duplicated elements, we will take the unique elements generated from approxQuantiles as input for Bucketizer.

How was this patch tested?

An unit test is added in QuantileDiscretizerSuite

QuantileDiscretizer.fit will throw an illegal exception when calling setSplits on a list of splits
with duplicated elements. Bucketizer.setSplits should only accept either a numeric vector of two
or more unique cut points, although that may produce less number of buckets than requested.

Signed-off-by: VinceShieh vincent.xie@intel.com

[srowen]

Hm, does .distinct guarantee that the ordering is preserved? It looks like it generally happens to. I guess we could add .sorted to make sure; it wouldn't be expensive, especially if it's already sorted

[srowen]

Jenkins add to whitelist

[VinceShieh]

yes, the output from approxQuantile is a sorted array.

[srowen]

No, my question is whether it remains sorted after .distinct. We'd need that, but I'm not sure it's guaranteed. It's easy to add .sorted to guarantee it.

[VinceShieh]

it seems Array.distinct will not break the sequence of the elements. But, you are right, we need guarantee the array is sorted.

[srowen]

Hm, I'm not sure why the tests write this. It shouldn't be needed. Leave it for now I guess

[srowen]

LGTM

[yanboliang]

I'm more prefer to leave warning log for this condition. Since if users set numBuckets = 10, and then get buckets with less than the numBuckets, it will confuse users. What about your thoughts? @srowen

[srowen]

I think that's reasonable, to warn in this case. It shouldn't be too noisy because this case is rare.

[SparkQA]

Test build #64184 has finished for PR 14747 at commit 4467bb6.

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

[srowen]

Nit: space after "if". Also this could state the number of resulting buckets, since it definitely will happen:

log.warn(s"Some quantiles were identical. Bucketing to ${splits.length - 1} buckets as a result.")

Finally, please update the title of the PR to match the JIRA, which is more descriptive as a commit log message.

[SparkQA]

Test build #64186 has finished for PR 14747 at commit d54b12c.

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

[SparkQA]

Test build #64196 has finished for PR 14747 at commit 5f414b7.

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

[SparkQA]

Test build #64188 has finished for PR 14747 at commit ea9146c.

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

[SparkQA]

Test build #64197 has finished for PR 14747 at commit 348436f.

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

[srowen]

Nit: you can use string interpolation here. That part was fine. s"... ${cutpoints.length - 1} ...". Super nit while we're still here: cutpoints is a term not used elsewhere. Just "distinctSplits"?

[SparkQA]

Test build #64323 has finished for PR 14747 at commit f800af2.

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

[srowen]

Merged to master