> On 2011-11-28 07:54:29, Ted Dunning wrote: > > Generally this looks like pretty clean code. Some more comments about > > intent would be nice. > > > > My review so far is very superficial.
I'm pretty blind to places which need more docs, as it all does, and I know the code. If you could point out places most in need of docs, I'll know where to start. :) > On 2011-11-28 07:54:29, Ted Dunning wrote: > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/LDADriver.java, > > line 217 > > <https://reviews.apache.org/r/2944/diff/1/?file=60151#file60151line217> > > > > Why return double? Main ignores this. Because any other program running this (currently: just the unit test I added) may want to know what the final converged perplexity was, so now it's available from the run() call. > On 2011-11-28 07:54:29, Ted Dunning wrote: > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0Driver.java, > > line 103 > > <https://reviews.apache.org/r/2944/diff/1/?file=60153#file60153line103> > > > > I think convention is @override on a seaparate line. Ah yes, I'll fix that. > On 2011-11-28 07:54:29, Ted Dunning wrote: > > trunk/core/src/main/java/org/apache/mahout/math/stats/Sampler.java, line 9 > > <https://reviews.apache.org/r/2944/diff/1/?file=60164#file60164line9> > > > > Javadoc here would be nice. Why is this sampler different from > > samplers we already have? > > > > Also, I don't see test code for this. I'll add documentation like follows: /** * Samples from a given discrete distribution: you provide a source of randomness and a Vector (cardinality N) which describes a distribution over [0,N), and calls to sample() sample from 0 to N * using this distribution */ Do we already have a sampler which does this? I can add tests, good point. > On 2011-11-28 07:54:29, Ted Dunning wrote: > > trunk/core/src/main/java/org/apache/mahout/math/stats/Sampler.java, line 58 > > <https://reviews.apache.org/r/2944/diff/1/?file=60164#file60164line58> > > > > So this looks like a multinomial sampler. Why not fit it into what > > already exists? Point me to the class! - Jake ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2944/#review3531 ----------------------------------------------------------- On 2011-11-27 20:37:25, Jake Mannix wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2944/ > ----------------------------------------------------------- > > (Updated 2011-11-27 20:37:25) > > > Review request for mahout. > > > Summary > ------- > > See MAHOUT-897 > > > This addresses bug MAHOUT-897. > https://issues.apache.org/jira/browse/MAHOUT-897 > > > Diffs > ----- > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/LDADriver.java > 1206835 > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0DocInferenceMapper.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0Driver.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0TopicTermVectorNormalizerMapper.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CachingCVB0Mapper.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CachingCVB0PerplexityMapper.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/InMemoryCollapsedVariationalBayes0.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/MemoryUtil.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/ModelTrainer.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/TopicModel.java > PRE-CREATION > trunk/core/src/main/java/org/apache/mahout/common/Pair.java 1206835 > > trunk/core/src/main/java/org/apache/mahout/math/DistributedRowMatrixWriter.java > PRE-CREATION > trunk/core/src/main/java/org/apache/mahout/math/MatrixUtils.java > PRE-CREATION > trunk/core/src/main/java/org/apache/mahout/math/stats/Sampler.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/mahout/clustering/ClusteringTestUtils.java > 1206835 > > trunk/core/src/test/java/org/apache/mahout/clustering/lda/TestMapReduce.java > 1206835 > > trunk/core/src/test/java/org/apache/mahout/clustering/lda/cvb/TestCVBModelTrainer.java > PRE-CREATION > trunk/src/conf/driver.classes.props 1206835 > > Diff: https://reviews.apache.org/r/2944/diff > > > Testing > ------- > > mvn clean test > > > Thanks, > > Jake > >
