4,5: I think its a question of correctness. For isEquals() the two vectors have to be exact. Otherwise clients should try to call something like isApproximatelyEquals method. For other places you have to check if Math.abs(map.apply(0.0) - 0.0) is exactly zero, again this has to be exact for the optimization to happen. Otherwise fallback on the slow method. No function that gives small numbers need to be optimized.
6) Yes examples of tests, you can choose anything. Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc. On Tue, Apr 30, 2013 at 11:07 AM, Dan Filimon <[email protected]>wrote: > 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? > > > > > >
