Pagerank empty matrix #1342
Pagerank empty matrix #1342
Conversation
There was a problem hiding this comment.
I like this PR, and think that the optional arguments for Pagerank are a clear improvement.
I feel like generally we adhere to the philosophy of keeping variables in the smallest scope possible and logically grouping declarations/assignments with their first usage, and find it strange that this file deviates so much from those practices. It's not a particularly important issue, though.
src/procedures/proc_pagerank.c
Outdated
| GrB_Descriptor_new(&desc); | ||
| GrB_Descriptor_set(desc, GrB_INP0, GrB_TRAN); | ||
| info = GrB_transpose(reduced, GrB_NULL, GrB_NULL, r, desc); | ||
| ASSERT(info == GrB_SUCCESS); |
| const int itermax = 100; // max iterations | ||
|
|
||
| GrB_Info info; | ||
| GrB_Index n = 0; // Node count |
There was a problem hiding this comment.
This initialization doesn't seem useful to me, as we'll always choose a real value for n at 101.
There was a problem hiding this comment.
It's important to set n to 0 here as it is later line 11 being used to initialise the PagerankContext
n notes the number of results returned from the actual PageRank algorithm.
src/procedures/proc_pagerank.c
Outdated
|
|
||
| GrB_Info info; | ||
| GrB_Index n = 0; // Node count | ||
| GrB_Index nvals; // Number of entries in 'reduced' |
There was a problem hiding this comment.
// Number of entries in 'r'
src/procedures/proc_pagerank.c
Outdated
| GrB_Matrix l = NULL; // Label matrix | ||
| GrB_Matrix r = NULL; // Relation matrix | ||
| GrB_Index *mapping = NULL; // Mapping, array for returning row indices of tuples | ||
| GrB_Matrix reduced = GrB_NULL; // Relation matrix reduced to only 'l' rows |
There was a problem hiding this comment.
reduced could be declared inside the if condition at 110.
| pdata->mapping = mapping; | ||
| pdata->ranking = ranking; |
There was a problem hiding this comment.
These will always be assigned at 165-166.
There was a problem hiding this comment.
Not if we quickly return at lines 83 and 91
| s = GraphContext_GetSchema(gc, label, SCHEMA_NODE); | ||
| // Unknown label, quickly return. | ||
| if(!s) return PROCEDURE_OK; | ||
| l = Graph_GetLabelMatrix(g, s->id); |
There was a problem hiding this comment.
I feel like it makes more sense to perform this work in the if(label) condition at 114.
There was a problem hiding this comment.
I prefer this notion of a "barrier", from this point onward we have both a label and relation matrices, which the next scope relies on.
| const double tol = 1e-4; // tolerance | ||
| const int itermax = 100; // max iterations | ||
|
|
||
| GrB_Info info; |
There was a problem hiding this comment.
Compiler warning in non-debug builds:
warning: variable ‘info’ set but not used
We should probably add an UNUSED macro to avoid these.
swilly22
force-pushed
the
pagerank-empty-matrix
branch
from
5f56322
to
89eed6c
Compare
* do not call pagerank on empty matrix * pagerank, optional arguments
#1340