-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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-7422][MLLIB] Add argmax to Vector, SparseVector #6112
Changes from 10 commits
04677af
3cffed4
df9538a
4526acc
eeda560
af17981
f21dcce
b1f059f
3ee8711
ee1a85a
d5b5423
ac53c55
aa330e3
f2eba2f
b22af46
42341fb
5fd9380
98058f4
2ea6a55
127dec5
3e0a939
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,6 +150,12 @@ sealed trait Vector extends Serializable { | |
toDense | ||
} | ||
} | ||
|
||
/** | ||
* Find the index of a maximal element. Returns the first maximal element in case of a tie. | ||
* Returns -1 if vector has length 0. | ||
*/ | ||
def argmax: Int | ||
} | ||
|
||
/** | ||
|
@@ -588,11 +594,7 @@ class DenseVector(val values: Array[Double]) extends Vector { | |
new SparseVector(size, ii, vv) | ||
} | ||
|
||
/** | ||
* Find the index of a maximal element. Returns the first maximal element in case of a tie. | ||
* Returns -1 if vector has length 0. | ||
*/ | ||
private[spark] def argmax: Int = { | ||
override def argmax: Int = { | ||
if (size == 0) { | ||
-1 | ||
} else { | ||
|
@@ -717,6 +719,53 @@ class SparseVector( | |
new SparseVector(size, ii, vv) | ||
} | ||
} | ||
|
||
override def argmax: Int = { | ||
if (size == 0) { | ||
-1 | ||
} else { | ||
|
||
var maxIdx = indices(0) | ||
var maxValue = values(0) | ||
|
||
foreachActive { (i, v) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not correct if all nonzeros are negative and there exist inactive (zero) entries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "there exist inactive (zero) entries" are you meaning we have values in the sparse vector that are zero but are set to not active? I thought that the sparse vector had you define what indices have active elements and that indices.size must equal the size of the values in that vector so I am not sure how you get into the state I think you are describing. Do you have an example? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait are you getting at say I have a sparse vector with zero in it and an assigned index? Like indices = (1,2,3) values = (-1,0,-.5)? If thats the case then yes I now see what you are saying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bug still exists. Take a sparse vector There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added unit tests to cover this and they appear to be passing so maybe I am totally misunderstanding the bug you are discussing? Or more likely I am misunderstanding the usage of the sparse vector implementation in regards to the inactive vs active elements? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I didn't explain this clearly. All inactive values in a sparse vector are zeros. The edge case here is that zero could be the max value of the entries. For example,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok yeah it sounds like I misunderstood the inactive vs active node concept. I'll get a fix in for that. I was assuming inactive meant we wanted to just ignore it and do the argmax over the active portions. |
||
if (v > maxValue) { | ||
maxIdx = i | ||
maxValue = v | ||
} | ||
} | ||
|
||
// look for inactive values in case all active node values are negative | ||
if(size != values.size && maxValue <= 0){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry to be that guy, but it looks like this would also fail our current convention that the first idx should be returned, if maxValues is zero and if the activeIndex that has a value zero is lesser than all inactive indices, something like.
It seems that argmax would return 1 in this case. (correct me if I'm wrong) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So are you thinking of the case where we have an inactive value thats set to something like 1? I dont think the api allows you to do that. My understanding of this case is that we will return idx=0 if 0 is the only max value found. Its technically correct since that active zero happens at the very beginning of the vector. I dont think we skip it due to the fact that someone decided to create a sparse vector with an active zero value. I am pretty sure i cover this case in my unit tests but I'll go back to the code real quick to double check. Also no worries. Better to find bugs than not right? lol. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had meant something like this.
Till this block of code, the maxIdx would be 0 and maxValue would be 0 and since the condition There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. Think I have this case handled as well now. Will push it up soon if it passes my unit tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems odd to allow addition of active nodes with value 0 if they should really be inactive. As well if we call SparseVector.toSparseVector it looks like it filters out the zeros to begin with so might make sense to do this more formally at object creation time. @mengxr thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Wouldn't this logic deal with all such cases?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I had a check for that in there but forgot I had reverted my code a bit. doh! realized it wasnt a new corner case anyways just the same one you saw earlier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw, scipy also allows zero values to be stored in the active nodes. Doing such a check might be expensive when values are large. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah good to know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space after if |
||
val firstInactiveIdx = calcFirstInactiveIdx(0) | ||
if(maxValue == 0){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. space after if; here and elsewhere |
||
if(firstInactiveIdx >= maxIdx) maxIdx else maxIdx = firstInactiveIdx | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can do away with the else here.
so that the only time when maxIdx does not change is when maxValue equals zero, and if firstInactiveIdx is greater than maxIdx |
||
}else{ | ||
maxIdx = firstInactiveIdx | ||
} | ||
maxValue = 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this line can be removed, since only maxIdx is used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I more kept it in there for clarity incase anyone is debugging through the code and can more easily understand what the associated idx and val are. But i can remove if its just too much clutter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not needed |
||
} | ||
maxIdx | ||
} | ||
} | ||
|
||
/** | ||
* Calculates the first instance of an inactive node in a sparse vector and returns the Idx | ||
* of the element. | ||
* @param idx starting index of computation | ||
* @return index of first inactive node | ||
*/ | ||
private[SparseVector] def calcFirstInactiveIdx(idx: Int): Int = { | ||
if (idx < size) { | ||
if (!indices.contains(idx)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
idx | ||
} else { | ||
calcFirstInactiveIdx(idx + 1) | ||
} | ||
} else { | ||
-1 | ||
} | ||
} | ||
} | ||
|
||
object SparseVector { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,11 +63,56 @@ class VectorsSuite extends FunSuite { | |
assert(vec.toArray.eq(arr)) | ||
} | ||
|
||
test("dense argmax"){ | ||
val vec = Vectors.dense(Array.empty[Double]).asInstanceOf[DenseVector] | ||
val noMax = vec.argmax | ||
assert(noMax === -1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: I think this can be combined into a single line, here and elsewhere
|
||
|
||
val vec2 = Vectors.dense(arr).asInstanceOf[DenseVector] | ||
val max = vec2.argmax | ||
assert(max === 3) | ||
|
||
val vec3 = Vectors.dense(Array(-1.0, 0.0, -2.0, 1.0)).asInstanceOf[DenseVector] | ||
val max2 = vec3.argmax | ||
assert(max === 3) | ||
} | ||
|
||
test("sparse to array") { | ||
val vec = Vectors.sparse(n, indices, values).asInstanceOf[SparseVector] | ||
assert(vec.toArray === arr) | ||
} | ||
|
||
test("sparse argmax"){ | ||
val vec = Vectors.sparse(0,Array.empty[Int],Array.empty[Double]).asInstanceOf[SparseVector] | ||
val noMax = vec.argmax | ||
assert(noMax === -1) | ||
|
||
val vec2 = Vectors.sparse(n,indices,values).asInstanceOf[SparseVector] | ||
val max = vec2.argmax | ||
assert(max === 3) | ||
|
||
val vec3 = Vectors.sparse(5,Array(2, 4),Array(1.0,-.7)) | ||
val max2 = vec3.argmax | ||
assert(max2 === 2) | ||
|
||
// check for case that sparse vector is created with only negative values {0.0, 0.0,-1.0, -0.7, 0.0} | ||
val vec4 = Vectors.sparse(5,Array(2, 3),Array(-1.0,-.7)) | ||
val max3 = vec4.argmax | ||
assert(max3 === 0) | ||
|
||
val vec5 = Vectors.sparse(11,Array(0, 3, 10),Array(-1.0,-.7,0.0)) | ||
val max4 = vec5.argmax | ||
assert(max4 === 1) | ||
|
||
val vec6 = Vectors.sparse(5,Array(0, 1, 3),Array(-1.0, 0.0, -.7)) | ||
val max5 = vec6.argmax | ||
assert(max5 === 1) | ||
|
||
var vec8 = Vectors.sparse(5,Array(1, 2),Array(0.0, -1.0)) | ||
val max7 = vec8.argmax | ||
assert(max7 === 0) | ||
} | ||
|
||
test("vector equals") { | ||
val dv1 = Vectors.dense(arr.clone()) | ||
val dv2 = Vectors.dense(arr.clone()) | ||
|
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.
Remove blank line.