giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Herald Kllapi" <heral...@gmail.com>
Subject Re: Review Request 13248: Added support for matrix aggregators. We use one aggregator per row and handle the aggregation of entries. The types supported are int, long, float, double.
Date Mon, 05 Aug 2013 18:24:14 GMT


> On Aug. 3, 2013, 12:03 a.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/Check.java, line
22
> > <https://reviews.apache.org/r/13248/diff/1/?file=334675#file334675line22>
> >
> >     You can use Guava's Preconditions instead of this.

Removed it :-)


> On Aug. 3, 2013, 12:03 a.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleMatrix.java,
line 42
> > <https://reviews.apache.org/r/13248/diff/1/?file=334676#file334676line42>
> >
> >     I would call this initialize().

done!


> On Aug. 3, 2013, 12:03 a.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleVector.java,
line 89
> > <https://reviews.apache.org/r/13248/diff/1/?file=334678#file334678line89>
> >
> >     I think this example is overkill. Just say it does element-by-element addition.

done!


> On Aug. 3, 2013, 12:03 a.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleVector.java,
line 96
> > <https://reviews.apache.org/r/13248/diff/1/?file=334678#file334678line96>
> >
> >     kv -> entry

done!


> On Aug. 3, 2013, 12:03 a.m., Alessandro Presta wrote:
> > giraph-core/src/test/java/org/apache/giraph/aggregators/matrix/InMemoryIO.java,
line 32
> > <https://reviews.apache.org/r/13248/diff/1/?file=334693#file334693line32>
> >
> >     I think you can use WritableUtils instead of this. Take a look at how TestVertexAndEdges
uses it.

done!


> On Aug. 3, 2013, 12:03 a.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/aggregators/matrix/DoubleVector.java,
line 40
> > <https://reviews.apache.org/r/13248/diff/1/?file=334678#file334678line40>
> >
> >     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.

I used Int2DoubleOpenHashMap.DEFAULT_INITIAL_SIZE instead of no arguments in order not to
have an if statement inside initialize or copy the code in the constructor. If you want I
can use the default constructor :-)


- Herald


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13248/#review24595
-----------------------------------------------------------


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message