[SPARK-8674] [MLlib] Implementation of a 2 sample Kolmogorov Smirnov Test

[josepablocam]

The current patch implements a 2-sample, 2-sided Kolmogorov Smirnov test. Similarly to the 1-sample implementation, we seek to reduce the shuffles necessary for computation. The user can provide 2 RDD[Double] and the Statistics.ksTest function allows them to test the null hypothesis that both samples came from the same probability distribution.

This patch includes the 1-sample test (so that reviewers can see the broader context of the change), however, that portion (and relevant tests) are being reviewed at https://issues.apache.org/jira/browse/SPARK-8598.

[srowen]

So this isn't to be merged in its current form? Put [WIP] in the title. This should probably just be reviewed later if/when the other PR is merged.

[sryza]

Hey @josepablocam can you rebase this on current master?

[josepablocam]

@sryza yes, will do.

[mengxr]

@sryza Do you want to make another pass? Please sign off if you think this is ready:)

[sryza]

Call this unionedData

[josepablocam]

changed

[srowen]

Would it be clearer to write true and false instead of isSample1 and !isSample1? I don't have a strong opinion.
Can the map functions be like .map((_, isSample1)) for tidiness or does that syntax not work here?
Finally I wonder if .union is clearer than ++ here? I don't have a strong opinion either, just am somehow used to method invocations on RDDs and ++-like syntax for Scala collections.

[sryza]

@mengxr @josepablocam oops thought it was still a WIP for some reason. Just took a pass. It looks mostly done - I just had a bunch of nits and a test request.

[josepablocam]

Mmm. I seem to be having some issues building and testing on my laptop. It keeps failing when building Catalyst. I'll try this first thing in the morning at work and push if it passes tests.

[srowen]

.sortByKey? I think the mapPartitions doesn't need braces, just parens, but that's tiny.

[sryza]

Nit: I'd make the above one "Sample follows the theoretical distribution" or make the bottom one "Both samples follow same distribution".

[josepablocam]

fixed. Modified the first. Thanks

[sryza]

jenkins, test this please

[sryza]

This LGTM pending jenkins

[mengxr]

test this please

[SparkQA]

Test build #39872 has finished for PR 7075 at commit 16ba96e.

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

[josepablocam]

ugh, did not reword the tests in pyspark after we slightly cleaned up the grammar in the 2 sample test. I will make the ks 2 sample test hypothesis statement match the grammar in the first. Sorry about this!

[sryza]

@mengxr is it too late to get this in to 1.5?

@josepablocam are you able to resolve merge conflicts?

[josepablocam]

@sryza fixed merge conflicts

[SparkQA]

Test build #58715 has finished for PR 7075 at commit feacda0.

• This patch fails Spark unit tests.
• This patch does not merge cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #69329 has finished for PR 7075 at commit feacda0.

• This patch fails Spark unit tests.
• This patch does not merge cleanly.
• This patch adds no public classes.

[HyukjinKwon]

@josepablocam, it looks the conflicts were not resolved cleanly. Would you resolve them?

[josepablocam]

I'm not involved in this anymore. If you're interested in making those fixes, feel free. Thanks.

…

On Jun 19, 2017 5:44 AM, "Hyukjin Kwon" ***@***.***> wrote: @josepablocam <https://github.com/josepablocam>, it looks the conflicts were not resolved cleanly. Would you resolve them? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7075 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE3GlO5eHVTNq47py-_XuJbhLTFkn89Eks5sFe6tgaJpZM4FNtLA> .

[gatorsmile]

We are closing it due to inactivity. please do reopen if you want to push it forward. Thanks!