I'm looking at the norm1 and times regressions again, maybe there's something I missed.
I agree with 1 through 3. About 4, 5, do you think we'd lose too much precision? About 6, you're giving examples of tests, not different special cases, right? As for the names, they're unfortunate, but I picked these after Ted said the initial ones (isLikeLeftMult was isLikeF0XEquals0...) were even worse. On Tue, Apr 30, 2013 at 7:01 PM, Robin Anil <[email protected]> wrote: > I see that the end is tantalizingly near. Few other review comments: > > 1) Remove all unused code. > 2) Do not allow construction of empty vectors. Just makes no sense (Unless > someone strongly disagrees). > 3) Comment all classes (AssignNonzerosIterateThisLookupThat etc). > 4) Change < Constants.EPSILON checks to == 0.0. > 5) Change > Constants.EPSILON check to !=0 ( I am sure you are using > Math.abs for such checks so it should be safe) > 6) Need some individual tests for each of > (AssignNonzerosIterateThisLookupThat etc. for toy examples, it might as > well be doing PLUS_ABS or MINUS for the entire thing) > > Apart from this I am not too happy with names of isLikeLeftMult, > isLikeRightPlus etc. But I dont have a good alternate either. Please run > this by Ted. > > > Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc. > > > On Tue, Apr 30, 2013 at 10:10 AM, Robin Anil <[email protected]> wrote: > > > Yes the incrementQuick is a known speed booster (due to half the number > of > > key hash generation). You can leave that to me. I can make it faster > after > > you check this in. It might require some refactor of the increment quick > > interface. > > > > What about the regressions in SeqSparseVector norm1? and > Dense.times(Seq)? > > Can you explain that? > > >
