Skip to content

[SPARK-2515][mllib] Chi Squared test #1733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dorx wants to merge 18 commits into from
Closed

[SPARK-2515][mllib] Chi Squared test #1733

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ff17423
WIP
dorx Jul 25, 2014
6598379
API and code structure.
dorx Jul 25, 2014
706d436
Added API for RDD[Vector]
dorx Jul 25, 2014
3d61582
input names
dorx Jul 25, 2014
e6b83f3
reviewer comments
dorx Aug 1, 2014
4e4e361
WIP
dorx Aug 2, 2014
50703a5
merge master
dorx Aug 2, 2014
bc7eb2e
unit passed; still need docs and some refactoring
dorx Aug 2, 2014
5686082
facelift
dorx Aug 6, 2014
d64c2fb
Merge branch 'master' into chisquare
dorx Aug 6, 2014
7eea80b
WIP
dorx Aug 7, 2014
e90d90a
Merge branch 'master' into chisquare
dorx Aug 7, 2014
c39eeb5
units passed with updated API
dorx Aug 7, 2014
80d03e2
Reviewer comments.
dorx Aug 8, 2014
7dde711
ChiSqTestResult renaming and changed to Class
dorx Aug 8, 2014
e95e485
reviewer comments.
dorx Aug 11, 2014
d286783
Merge branch 'master' into chisquare
dorx Aug 11, 2014
cafb3a7
fixed p-value for extreme case.
dorx Aug 11, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
reviewer comments
  • Loading branch information
@dorx
dorx committed Aug 1, 2014
commit e6b83f35375701f71f699697a83236e7e0c76d6c
10 changes: 5 additions & 5 deletions mllib/src/main/scala/org/apache/spark/mllib/stat/test/TestResult.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ trait TestResult {
* Specific classes implementing this trait should override this method to output test-specific
* information.
*/
def explain: String = {
s"pValue = $pValue \n" +
override def toString: String = {
s"pValue = $pValue \n" + // TODO explain what pValue is
s"degrees of freedom = $degreesOfFreedom \n" +
s"statistic = $statistic"
}
Expand All @@ -47,14 +47,14 @@ trait TestResult {
* :: Experimental ::
*/
@Experimental
class ChiSquaredTestResult(override val pValue: Double,
case class ChiSquaredTestResult(override val pValue: Double,
Copy link
Contributor

@mengxr mengxr Aug 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need to be a case class? Scala compiler will add many methods to a case class and make it very hard to extend.

Copy link
Contributor Author

@dorx dorx Aug 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case class is a logical choice here since it's essentially an immutable object holding a bunch of invariant fields and doesn't do any stateful computations inside of the class. Is there development plan for extending this classes in the future?

Copy link
Contributor

@mengxr mengxr Aug 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No case class features are used, especially pattern matching. This case class will extend Product5 and make it impossible to add a field, for example, whether correction is used or not. Also, with a case class, it is very hard to add a static method. We might want to write the test result to JSON and later parse it back. A natural choice would be ChiSquaredTestResult.fromJSON(json: String) but it is very complicated to match the type signature generated by Scala's compiler. We had this problem with LabeledPoint in MLlib, which is a public case class.

Copy link
Contributor

@mengxr mengxr Aug 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, shall we rename it to ChiSqTestResult? So chiSqTest() returns ChiSqTestResult.

Copy link
Contributor Author

@dorx dorx Aug 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether correction is used or not can actually be reflected in the method name (pearson v yates). I doubt there's a lot of use cases for parsing the result back from JSON so let's not worry about it for now (also, don't static methods usually come in the companion object anyway?). The way I see case classes is that they're like data structs that encapsulates immutable fields (the list of fields can be modified in later releases given that this is all experimental) and we get free getter methods for constructor arguments with a case class, but if there are compiler optimization complications, I can change it to a regular class.

override val degreesOfFreedom: Array[Int],
override val statistic: Double,
val method: String) extends TestResult {

override def explain: String = {
override def toString: String = {
"Chi squared test summary: \n" +
s"method: $method \n" +
super.explain
super.toString
}
}