Ignore that, I found an issue with the Dense iterator. Those test are passing now except for one(testTimesSquaredTimesVector(org.apache.mahout.math.PivotedMatrixTest)). i have also updated the review request.
https://reviews.apache.org/r/10455/diff/#index_header On Sun, Apr 14, 2013 at 8:13 PM, Robin Anil <[email protected]> wrote: > 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) >>> > >> > > > > >>> > >> > > > >>> > >> > > >>> > >> > >>> > >> >>> > > >>> > > >>> > >>> >> >> >
