-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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.
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
[SPARK-8479] [MLlib] Add numNonzeros and numActives to linalg.Matrices #6904
Conversation
Test build #35267 has finished for PR 6904 at commit
|
Test build #35282 has finished for PR 6904 at commit
|
@@ -176,6 +176,12 @@ object MimaExcludes { | |||
ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.sql.jdbc.PostgresQuirks"), | |||
ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.sql.jdbc.NoQuirks"), | |||
ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.sql.jdbc.MySQLQuirks") | |||
) ++ Seq( | |||
// SPARK-8479 Add numNonzeros and numActives to Matrix. | |||
ProblemFilters.exclude[MissingClassProblem]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be "MissingMethodProblem" instead?
@mengxr Sorry about that. Btw, when is a foreach preferred as compared to a while loop. |
Test build #35286 has finished for PR 6904 at commit
|
Test build #35291 has finished for PR 6904 at commit
|
ping @srowen Could you please help me figure out the compatibility problems here? I've added it to the filters but it does not seem to work. |
@@ -592,6 +614,19 @@ class SparseMatrix( | |||
def toDense: DenseMatrix = { | |||
new DenseMatrix(numRows, numCols, toArray) | |||
} | |||
|
|||
override def numNonzeros: Int = { | |||
var nnz = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this not be values.count(_ != 0)
?
I think the failure is indeed because you're adding methods to a trait, but it's a sealed trait, so probably fine to override. I think you put it in the wrong place? it shoudl go under 1.5 |
Test build #36277 has finished for PR 6904 at commit
|
Test build #36278 has finished for PR 6904 at commit
|
Thanks for the comments but Vector is a sealed trait as well but that seems to work fine. |
MiMa doesn't know it. So we still need to add the exclusion rules explicitly, otherwise it won't pass Jenkins. |
Have I done it correctly? |
Test build #36288 has finished for PR 6904 at commit
|
LGTM. Merged into master. Thanks! |
Matrices allow zeros to be stored in values. Sometimes a method is handy to check if the numNonZeros are same as number of Active values.