Sure thing. Your comments will be applied to the whole patch. On Wednesday, July 31, 2013, Sebastian Schelter wrote:
> Looking forward to your modifications! Please note, that I marked some > things only once (whitespaces after if, lowercase method names), but I > would be great if you could fix that in the whole patch. > > Best, > Sebastian > > On 01.08.2013 07:55, Michael Kun Yang wrote: > > Hi Sebastian, > > > > Thank you very much for going through the code. I will modify the code > > accordingly. > > > > On Wednesday, July 31, 2013, Sebastian Schelter wrote: > > > >> This is an automatically generated e-mail. To reply, visit: > >> https://reviews.apache.org/r/13172/ > >> > >> Overall, this looks very good. I can't say much to the algorithmic > details as I'm not familiar with them. The code is high quality but needs > to adhere more to our coding conventions and use more of the tools that > Mahout already offers like adding options to jobs and configuring them. > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearConstants.java< > https://reviews.apache.org/r/13172/diff/1/?file=331599#file331599line20> > (Diff > >> revision 1) > >> > >> 20 > >> > >> public class PenalizedLinearConstants { > >> > >> please add a private constructor to ensure that this class cannot be > instantiated. > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearDriver.java< > https://reviews.apache.org/r/13172/diff/1/?file=331600#file331600line103> > (Diff > >> revision 1) > >> > >> 103 > >> > >> if(parseArgs(args)) { > >> > >> insert a whitespace between if and the opening bracket > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearDriver.java< > https://reviews.apache.org/r/13172/diff/1/?file=331600#file331600line111> > (Diff > >> revision 1) > >> > >> 111 > >> > >> PrintInfo(parameter, solver, "path"); > >> > >> method names should always start lowercase in Java > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearDriver.java< > https://reviews.apache.org/r/13172/diff/1/?file=331600#file331600line130> > (Diff > >> revision 1) > >> > >> 130 > >> > >> String line = "" + String.format("%" + > Integer.toString(formatWidth) + ".5f", lambdas[i]); > >> > >> use a StringBuilder for concatenating strings in loops > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearDriver.java< > https://reviews.apache.org/r/13172/diff/1/?file=331600#file331600line137> > (Diff > >> revision 1) > >> > >> 137 > >> > >> } > >> > >> else should be on the same line as the closing bracket of if > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearDriver.java< > https://reviews.apache.org/r/13172/diff/1/?file=331600#file331600line181> > (Diff > >> revision 1) > >> > >> 181 > >> > >> Job job = new Job(conf, "Penalized Linear Regression Driver running > over input: " + input); > >> > >> please check whether you can use AbstractJob's prepareJob() method to > configure the job > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearDriver.java< > https://reviews.apache.org/r/13172/diff/1/?file=331600#file331600line203> > (Diff > >> revision 1) > >> > >> 203 > >> > >> private boolean parseArgs(String[] args) { > >> > >> try to use the addOption(...) methods from AbstractJob > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearKeySet.java< > https://reviews.apache.org/r/13172/diff/1/?file=331601#file331601line20> > (Diff > >> revision 1) > >> > >> 20 > >> > >> public class PenalizedLinearKeySet { > >> > >> again, add a private constructor > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearMapper.java< > https://reviews.apache.org/r/13172/diff/1/?file=331602#file331602line41> > (Diff > >> revision 1) > >> > >> 41 > >> > >> HashMap<String, double[]> rowMatrix; > >> > >> use Map<> not HashMap<> as type > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearMapper.java< > https://reviews.apache.org/r/13172/diff/1/?file=331602#file331602line48> > (Diff > >> revision 1) > >> > >> 48 > >> > >> rowMatrix = new HashMap<String, double[]>(); > >> > >> please use Maps.newHashMap() to create the map > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearMapper.java< > https://reviews.apache.org/r/13172/diff/1/?file=331602#file331602line52> > (Diff > >> revision 1) > >> > >> 52 > >> > >> log.error("Fatal: Input file format is wrong!", new > IOException("Fatal: Input file format is wrong!")); > >> > >> You have to throw an Exception here, not only log it > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearMapper.java< > https://reviews.apache.org/r/13172/diff/1/?file=331602#file331602line62> > (Diff > >> revision 1) > >> > >> 62 > >> > >> String emitKey = String.valueOf(dimension) + "." + > Integer.toString(rand.nextInt(numberOfCV)); > >> > >> Why Text as Key? Could you not use a writable with two integers? > >> > >> > >> > >> > core/src/main/java/org/apache/mahout/regression/penalizedlinear/PenalizedLinearReducer.java< > https://reviews.apache.org/r/13172/diff/1/?file=331603#file331603line41> > (Diff > >> revision 1) > >> > >> 41 > >> > >> valueLen = (5 * dimension + dimension * dimension) / 2 + 3; > >> > >> please add a comment explaining the calculation > >> > >> > >> > >> < > https://reviews.apache.org/r/13172/diff/1/?file=331603#file331603line44> > >> > > > >
