----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13248/#review24595 -----------------------------------------------------------
Looking pretty good. I made a few comments on the Double case, but they obviously apply to all versions. giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/Check.java <https://reviews.apache.org/r/13248/#comment48621> You can use Guava's Preconditions instead of this. giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleMatrix.java <https://reviews.apache.org/r/13248/#comment48622> I would call this initialize(). giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleVector.java <https://reviews.apache.org/r/13248/#comment48626> Why do we initialize to 0 length in absence of hints? This can lead to a suboptimal reallocation pattern. I would stick with Fastutil's default, by constructing the hash map with no arguments. giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleVector.java <https://reviews.apache.org/r/13248/#comment48625> I think this example is overkill. Just say it does element-by-element addition. giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleVector.java <https://reviews.apache.org/r/13248/#comment48624> kv -> entry giraph-core/src/test/java/org/apache/giraph/aggregators/matrix/InMemoryIO.java <https://reviews.apache.org/r/13248/#comment48628> I think you can use WritableUtils instead of this. Take a look at how TestVertexAndEdges uses it. - Alessandro Presta On Aug. 2, 2013, 11:07 p.m., Herald Kllapi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13248/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2013, 11:07 p.m.) > > > Review request for giraph. > > > Repository: giraph-git > > > Description > ------- > > In applications where a matrix is needed, is not efficient to have an > aggregator per entry. This update provides the same functionality with an > aggregator per matrix row. > > > Diffs > ----- > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/Check.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleMatrix.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleMatrixSumAggregator.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleVector.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleVectorSumAggregator.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/FloatMatrix.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/FloatMatrixSumAggregator.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/FloatVector.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/FloatVectorSumAggregator.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/IntMatrix.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/IntMatrixSumAggregator.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/IntVector.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/IntVectorSumAggregator.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/LongMatrix.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/LongMatrixSumAggregator.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/LongVector.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/LongVectorSumAggregator.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/package-info.java > PRE-CREATION > > giraph-core/src/test/java/org/apache/giraph/aggregators/matrix/InMemoryIO.java > PRE-CREATION > > giraph-core/src/test/java/org/apache/giraph/aggregators/matrix/TestDoubleMatrix.java > PRE-CREATION > > giraph-core/src/test/java/org/apache/giraph/aggregators/matrix/TestFloatMatrix.java > PRE-CREATION > > giraph-core/src/test/java/org/apache/giraph/aggregators/matrix/TestIntMatrix.java > PRE-CREATION > > giraph-core/src/test/java/org/apache/giraph/aggregators/matrix/TestLongMatrix.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/13248/diff/ > > > Testing > ------- > > We provide test classes to test the functionality. > > > Thanks, > > Herald Kllapi > >
