----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2944/#review3531 -----------------------------------------------------------
Generally this looks like pretty clean code. Some more comments about intent would be nice. My review so far is very superficial. trunk/core/src/main/java/org/apache/mahout/clustering/lda/LDADriver.java <https://reviews.apache.org/r/2944/#comment7864> Why return double? Main ignores this. trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0Driver.java <https://reviews.apache.org/r/2944/#comment7865> I think convention is @override on a seaparate line. trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CachingCVB0PerplexityMapper.java <https://reviews.apache.org/r/2944/#comment7866> This comment is very confusing. trunk/core/src/main/java/org/apache/mahout/math/stats/Sampler.java <https://reviews.apache.org/r/2944/#comment7867> Javadoc here would be nice. Why is this sampler different from samplers we already have? Also, I don't see test code for this. trunk/core/src/main/java/org/apache/mahout/math/stats/Sampler.java <https://reviews.apache.org/r/2944/#comment7868> So this looks like a multinomial sampler. Why not fit it into what already exists? - Ted 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 > >
