After fixing iterator(assuming my patch is correct). The following tests are failing in math because of incorrect usage of next() without checking hasNext(). I would need some help in fixing them as I have never touched the code before
SequentialBigSvdTest.testSingularValues:40->assertEquals:64 » NullPointer LogLikelihoodTest.testFrequencyComparison:108 » NullPointer TestHebbianSolver.testHebbianSolver:86->timeSolver:59 » NullPointer MultiNormalTest.testDiagonal:54 » NullPointer PermutedVectorViewTest.testIterators:60 » NullPointer WeightedVectorTest.testProjection:66 » NullPointer WeightedVectorTest>AbstractVectorTest.testSimpleOps:52 » NullPointer See the iterator patch https://reviews.apache.org/r/10455/diff/#index_header Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc. On Sun, Apr 14, 2013 at 7:27 PM, Robin Anil <[email protected]> wrote: > I am working on a patch. Here is a sample. This one is for RASV. I put > Dan's example as a test case. > > > > 1. private final class NonDefaultIterator implementsIterator<Element> { > 2. private final class NonDefaultElement implements Element { > 3. @Override > 4. public double get() { > 5. return mapElement.get(); > 6. } > 7. > 8. @Override > 9. public int index() { > 10. return mapElement.index(); > 11. } > 12. > 13. @Override > 14. public void set(double value) { > 15. invalidateCachedLength(); > 16. mapElement.set(value); > 17. } > 18. } > 19. > 20. private final NonDefaultElement element = newNonDefaultElement(); > 21. private final Iterator<MapElement> iterator; > 22. private MapElement mapElement; > 23. > 24. private NonDefaultIterator() { > 25. this.iterator = values.iterator(); > 26. } > 27. > 28. @Override > 29. public boolean hasNext() { > 30. return iterator.hasNext(); > 31. } > 32. > 33. @Override > 34. public Element next() { > 35. mapElement = iterator.next(); > 36. return element; > 37. } > 38. > 39. @Override > 40. public void remove() { > 41. throw new UnsupportedOperationException(); > 42. } > 43. } > 44. > > > > Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc. > > > On Sun, Apr 14, 2013 at 7:04 PM, Ted Dunning <[email protected]>wrote: > >> Well... current iterator style with a non-side-effecting version of >> hasNext(), of course. >> >> Reusing the container is OK if the performance hit is substantial. >> >> >> On Sun, Apr 14, 2013 at 5:02 PM, Robin Anil <[email protected]> wrote: >> >> > Also the Tests crash due to excessive GC. The performance degradation >> there >> > is very visible(my cpu spikes up). I think there is good case for the >> > current iteration style, just that we have to, not use the java Iterator >> > contract and confuse clients. >> > >> > Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc. >> > >> > >> > On Sun, Apr 14, 2013 at 6:59 PM, Robin Anil <[email protected]> >> wrote: >> > >> > > Yes. All final. >> > > >> > > Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc. >> > > >> > > >> > > On Sun, Apr 14, 2013 at 6:55 PM, Ted Dunning <[email protected] >> > >wrote: >> > > >> > >> Did you mark the class and fields all as final? >> > >> >> > >> That might help the compiler realize it could in-line stuff and avoid >> > the >> > >> constructor (not likely, but possible) >> > >> >> > >> >> > >> On Sun, Apr 14, 2013 at 4:52 PM, Robin Anil <[email protected]> >> > wrote: >> > >> >> > >> > With a new immutable Element in the iterator, the iteration >> behavior >> > is >> > >> > corrected but. There is a performance degradation of about 10% and >> > >> > nullifies what I have done with the patch. >> > >> > >> > >> > See >> > >> > >> > >> > >> > >> >> > >> https://docs.google.com/spreadsheet/ccc?key=0AhewTD_ZgznddGFQbWJCQTZXSnFULUYzdURfWDRJQlE#gid=1 >> > >> > >> > >> > Robin Anil | Software Engineer | +1 312 869 2602 | Google Inc. >> > >> > >> > >> > >> > >> > On Sun, Apr 14, 2013 at 11:28 AM, Ted Dunning < >> [email protected]> >> > >> > wrote: >> > >> > >> > >> > > Yeah... but we still have to fix the iterator. >> > >> > > >> > >> > > >> > >> > > On Sun, Apr 14, 2013 at 8:58 AM, Robin Anil < >> [email protected]> >> > >> > wrote: >> > >> > > >> > >> > > > Here is an iteration style that works as is with today's >> behaviour >> > >> of >> > >> > > > hasNext >> > >> > > > >> > >> > > > 1. >> > >> > > > 2. Element thisElement = null; >> > >> > > > 3. Element thatElement = null; >> > >> > > > 4. boolean advanceThis = true; >> > >> > > > 5. boolean advanceThat = true; >> > >> > > > 6. >> > >> > > > 7. Iterator<Element> thisNonZero = >> this.iterateNonZero(); >> > >> > > > 8. Iterator<Element> thatNonZero = x.iterateNonZero(); >> > >> > > > 9. >> > >> > > > 10. double result = 0.0; >> > >> > > > 11. while (true) { >> > >> > > > 12. *if (advanceThis) { >> > >> > > > * >> > >> > > > 13. * if (!thisNonZero.hasNext()) { >> > >> > > > * >> > >> > > > 14. * break; >> > >> > > > * >> > >> > > > 15. * } >> > >> > > > * >> > >> > > > 16. * thisElement = thisNonZero.next(); >> > >> > > > * >> > >> > > > 17. * } >> > >> > > > * >> > >> > > > 18. * if (advanceThat) { >> > >> > > > * >> > >> > > > 19. * if (!thatNonZero.hasNext()) { >> > >> > > > * >> > >> > > > 20. * break; >> > >> > > > * >> > >> > > > 21. * } >> > >> > > > * >> > >> > > > 22. * thatElement = thatNonZero.next(); >> > >> > > > * >> > >> > > > 23. * }* >> > >> > > > 24. if (thisElement.index() == thatElement.index()) >> { >> > >> > > > 25. >> > >> > > > 26. result += thisElement.get() * >> thatElement.get(); >> > >> > > > 27. advanceThis = true; >> > >> > > > 28. advanceThat = true; >> > >> > > > 29. } else if (thisElement.index() < >> > >> thatElement.index()) { >> > >> > > > 30. advanceThis = true; >> > >> > > > 31. advanceThat = false; >> > >> > > > 32. } else { >> > >> > > > 33. advanceThis = false; >> > >> > > > 34. advanceThat = true; >> > >> > > > 35. } >> > >> > > > 36. } >> > >> > > > >> > >> > > > >> > >> > > > On Sat, Apr 13, 2013 at 1:47 AM, Ted Dunning < >> > [email protected] >> > >> > >> > >> > > > wrote: >> > >> > > > >> > >> > > > > The caller is not at fault here. The problem is that >> hasNext is >> > >> > > > advancing >> > >> > > > > the iterator due to a side effect. The side effect is >> > impossible >> > >> to >> > >> > > > avoid >> > >> > > > > at the level of the caller. >> > >> > > > > >> > >> > > > > Sent from my iPhone >> > >> > > > > >> > >> > > > > On Apr 12, 2013, at 12:22, Sean Owen <[email protected]> >> wrote: >> > >> > > > > >> > >> > > > > > I'm sure I did (at least much of) the AbstractIterator >> change >> > so >> > >> > > blame >> > >> > > > > > me... but I think the pattern itself is just fine. It's >> used >> > in >> > >> > many >> > >> > > > > > places in the project. Reusing the value object is a big >> win >> > in >> > >> > some >> > >> > > > > > places. Allocating objects is fast but a trillion of them >> > still >> > >> > adds >> > >> > > > > > up. >> > >> > > > > > >> > >> > > > > > It does contain a requirement, and that is that the caller >> is >> > >> > > supposed >> > >> > > > > > to copy/clone the value if it will be used at all after the >> > next >> > >> > > > > > iterator operation. That's the 0th option, to just fix the >> > >> caller >> > >> > > > > > here. >> > >> > > > > > >> > >> > > > > > On Fri, Apr 12, 2013 at 7:49 PM, Ted Dunning < >> > >> > [email protected]> >> > >> > > > > wrote: >> > >> > > > > >> The contract of computeNext is that there are no side >> effects >> > >> > > visible >> > >> > > > > >> outside (i.e. apparent functional style). This is >> required >> > >> since >> > >> > > > > >> computeNext is called from hasNext(). >> > >> > > > > >> >> > >> > > > > >> We are using a side-effecting style so we have a bug. >> > >> > > > > >> >> > >> > > > > >> We have two choices: >> > >> > > > > >> >> > >> > > > > >> a) use functional style. This will *require* that we >> > allocate a >> > >> > new >> > >> > > > > >> container element on every call to computeNext. This is >> best >> > >> for >> > >> > > the >> > >> > > > > user >> > >> > > > > >> because they will have fewer surprising bugs due to reuse. >> > If >> > >> > > > > allocation >> > >> > > > > >> is actually as bad as some people think (I remain >> skeptical >> > of >> > >> > that >> > >> > > > > without >> > >> > > > > >> tests) then this is a bad move. If allocation of totally >> > >> > ephemeral >> > >> > > > > objects >> > >> > > > > >> is as cheap as I think, then this would be a good move. >> > >> > > > > >> >> > >> > > > > >> b) stop using AbstractIterator and continue with the >> re-use >> > >> style. >> > >> > > > And >> > >> > > > > add >> > >> > > > > >> a comment to prevent a bright spark from reverting this >> > change. >> > >> > (I >> > >> > > > > suspect >> > >> > > > > >> that the bright spark who did this in the first place was >> me >> > >> so I >> > >> > > can >> > >> > > > be >> > >> > > > > >> rude) >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > >> >> > > >> > > >> > >> > >
