Two mistakes in my view

[harryhan618]

Hello, thanks for the open-sourced code! During reading the code, there are two places that I don't understand.

1. At this line, I think num_col_partitions should be assigned rather than num_row_partitions.

2. During running bench_vanilla_sddmm.py, I notice that data in reddit_coo_float32.npz is not sorted. That is to say, in loaded COO matrix, for every source vertex, its destination vertices index are not sorted. I don't quite understand why. Is it a preprocessed adjacency matrix by reorderding the vertices? And I'm not sure whether it may cause some issue when indexing CSR matrix in the graph partition procedure?

[yzh119]

For 1, yes you are right, thanks for pointing out.
For 2, the loaded COO matrix is not sorted, and for SDDMM the partition algorithm (https://github.com/amazon-research/FeatGraph/blob/e06792cc61ae39500cf7c40979d6c394f53ee362/python/featgraph/util/adj_partitioning.py#L60) do not assume the input COO matrix is sorted, CSR is not required for SDDMM.

[harryhan618]

For 2, but for util_partition_adj_coo_2d as the SDDMMbase preprocess function, COO is converted to CSR first. Then graph partition is done in two for-loops, tiles one by one. In the two for-loop, the code directly index into the CSR matrix. Also, there's a line adj_scipy_coo.data = np.arange(1, 1 + nnz, dtype='int32') as non zero indices, which is not sorted because of the loaded matrix. Thus, indexing CSR is not sorted as well. So I don't think indexing the CSR matrix is actually doing the right thing.

So I would like to know what is your intention during graph partition. Does it mean partitioning the normal adjacent matrix tile by tile? And why the loaded COO file is not sorted while the other CSR file is sorted?

Thanks!

[Huyuwei]

@harryhan618 Thanks for pointing it out!
The COO matrix should be sorted. The results reported in the paper are tested on sorted coo matrices. 2D partitioning improves locality within each partition, thus bringing speedup. Sorry that I was not aware that the coo data loaded from dgl by default is not sorted. I have filed a PR to solve it. #3

[yzh119]

Sorry about the confusion, I'm not aware that coo is converted to csr in the partition process, @Huyuwei 's new PR fix the problem.

[harryhan618]

Thank you for the reply!