I dunno man, I think we've found 2 errors so far because the dev glibly did the simplest possible thing and did for (Element e : vector), and they really meant for (Element e : vector.nonZeroes()).
My thinking is this: if you only have one kind of iterator, then it clearly makes sense to implement Iterable. If you provide two different iterators, neither of which is clearly the right one, then don't implement Iterable, as it shows preference for one iterator. In this case, it actually shows preference for the *wrong* iterator. On Thu, May 23, 2013 at 7:33 PM, Ted Dunning <[email protected]> wrote: > Looks good to me. > > I still don't like the all() usage. > > Can you leave the interface in place on the vector so one has the choice of > idioms? > > > > On Thu, May 23, 2013 at 7:06 PM, Jake Mannix <[email protected]> > wrote: > > > It's done, patch passes tests, but it involves adding 308 lines > > (removing 556 others!) across 84 classes in the codebase, so it probably > > touches something you wrote. > > > > So I'm going to let this diff marinade for a day or so, but if you feel > > like you've got some thoughts (+1's? -1's? I'll even take some > +epsilons > > if you're feeling cowardly) on it, feel free to comment on the > > ticket<https://issues.apache.org/jira/browse/MAHOUT-1227> or > > on the review <https://reviews.apache.org/r/11359> itself. > > > > I won't let the patch stick around forever though, as most every commit > > that hits trunk will force me to regenerate this patch again, given its > > breadth. > > > > -- > > > > -jake > > > -- -jake
