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> >
