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